-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
V1.1.x issue 98 #100
V1.1.x issue 98 #100
Conversation
… ending in 99 still work
Hi @spangaer, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Only minor changes request. Adding the "high-distrust" fixes option is great. Nice touch!
Once the nitpicks are fixed and I've considered this a bit more (while fixing CI) I'll approve and merge. :)
defaultPublishTo := crossTarget.value / "repository", | ||
releasePublishArtifactsAction := PgpKeys.publishSigned.value | ||
) | ||
defaultPublishTo := crossTarget.value / "repository") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove change from PR? Or is this no longer required with the updated sbt-pgp plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I don't know. I just made an effort to make it build. I haven't actually done PGP signed publishes.
This part specifically I stumbled on in this commit:
akka/akka-persistence-dynamodb@0192e14
So it looks like it can do without.
- new line - remove 3 files
Hi @spangaer, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement: |
On the subject of the CLA, I'm looking in to that. I was waiting for a bit to see if the PR was going to be picked up. I'll get it taken care of ASAP, but I need to pick up it up internally. I know I'm not out of my jurisdiction by simply pushing it to a public repo, but the CLA adds an extra level. (but I understand that's where copyright history has landed us) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM tho you should wait until I fix CI to merge in master.
Also update the test to use `IntegSpec. This incorporates the container based DynamoDB testing.
I haven't forgotten about this. Still waiting for the legal department... |
CLA part fixed. |
@lightbend-cla-validator please check again |
Hi @spangaer thanks for poking the CLA bot. |
Hi Any idea when a new release would be made including this fix? |
@jvce This was included in |
#98
Hi, I'd actually like to submit this pull request against a 1.1.x version. So it could potentially be released as 1.1.2.
If there's interest and there's an intention of releasing an actual 1.2.0. I guess we can look in to porting to that too.
Any feedback is welcome.
For a more detailed explanation on how things are fixed please read here:
akka/akka-persistence-dynamodb#98 (comment)