-
Notifications
You must be signed in to change notification settings - Fork 25
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
Ability to set arbitrary tags for the image #7
Conversation
Thanks for the contribution @mikegirkin. Before we merge this, I'd like to understand the use-case a bit better, as I feel 2 attributes are somehow redundant in this scope.
If you skip the Regarding the plugin contract and the proposed change:
I think I need to think this through as all options seem to have good arguments for and against. -- |
@sbilinski My usecase is exactly the one you described. I want to have exact version marks, as well as "latest" tag, to refer to the latest release. |
@mikegirkin Thanks for the explanation - makes perfect sense. Frankly speaking, I did not need it myself, since we assumed a roll over policy at this stage of process and a single tag was sufficient so far. That being said, I think multi-push would fit well within the scope of this plugin:
I'm leaning towards option 3, which would involve removing
That way, we could write everything in one place:
I'd keep My reasoning is as follows:
Thoughts? |
@sbilinski I am totally okay with that, as it was close to my initial idea, but when I did that PR I just didn't want to do that kind of a breaking change. I'll do that shortly |
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.
Looks good - thanks! I just have one minor remark regarding the old setting (see comments).
@@ -25,7 +26,7 @@ object EcrPlugin extends AutoPlugin { | |||
override lazy val projectSettings = inConfig(ecr)(defaultSettings ++ tasks) | |||
|
|||
lazy val defaultSettings: Seq[Def.Setting[_]] = Seq( | |||
version := "latest", |
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 think we can safely remove import sbt.Keys.version
after removing this setting. Also, please update the README.md
file as it still references version
in the example.
With the release of 0.5.0 the image marked as "latest" only when the version is not provided in ecr.
Here is the PR that allows putting arbitrary tags to the image, i.e. "latest"