-
Notifications
You must be signed in to change notification settings - Fork 443
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
Use SNAPSHOT for release when snapshot version #995
Conversation
Hi @keirlawson, 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: |
I have signed the Lightbend CLA |
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 for fixing this :)
Implementation LGTM. I added some small remarks.
|
||
rpmLicense := Some("BSD") | ||
|
||
|
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.
A simple test task checking the rpmVersion
would be nice. E.g. something like
TaskKey[Unit]("unzip") := {
assert(rpmRelease.value === "SNAPSHOT", s"RPM has incorrect value ${rpmVersion.value}")
assert(rpmMetadata.value.version === "...", s"RPM has incorrect value ${rpmMetadata.value.version}")
()
}
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.
Done
@@ -59,11 +59,16 @@ object RpmPlugin extends AutoPlugin { | |||
|
|||
} | |||
|
|||
private def stripSnapshot(version: String): String = { |
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.
I would put the private methods at the bottom of the AutoPlugin to preserve a "general -> detailed" structure :)
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.
Done
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.
Done
Have moved helper method and added tests as suggested. |
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.
Awesome 😍
Fixes #971 - feedback appreciated!