Skip to content
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

[Docker] add multiple docker tags support #1138

Merged
merged 13 commits into from
Jul 13, 2018
Merged

Conversation

kimxogus
Copy link
Contributor

Added additional docker image tags support.

Before, if your project's version is 1.1.0 and you want to tag 1.1 and 1, you had to tag 1.1 and 1 manually outside of sbt task. With this PR, you can specify additional docker image tags and manage them in sbt.

@sbt sbt deleted a comment Jul 12, 2018
@sbt sbt deleted a comment Jul 12, 2018
@kimxogus kimxogus force-pushed the docker/tags branch 2 times, most recently from 033260f to e93ed5e Compare July 12, 2018 10:54
Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for your awesome work on the docker plugin 😍

The general implementation idea is great. Defining a container dockerAliases, which contains all aliases to be built. I would actually like to cut it down to this setting without introducing any another settings. The reason is that any new setting needs to be maintained (managed, documented, etc.)

I would suggest the following way:

  1. Keep the dockerAlias setting as the "primary docker alias". We use this as the starting point for other aliases
  2. Add helper methods on the DockerAlias class for a more readable API, e.g. withTag / withUntagged, withusername

For users this would look like this

dockerAliases += dockerAlias.value.withUntagged(),
dockerAliases += dockerAlias.value.withTag("foo")

WDYT?

@kimxogus
Copy link
Contributor Author

That sounds much better. That would be more intuitive. I'll change them

@kimxogus
Copy link
Contributor Author

  • Updated DockerAlias with more readable member methods
  • Updated sbt-scalafmt because it reformats differently with Intellij's scalafmt.

Copy link
Contributor

@muuki88 muuki88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the changes 🎉 This looks smooth an clean.

I'll merge once the build is green.

Also thanks for upgrading scalafmt. Normally I would like to have such changes in a separate PR to be able to squash and merge it more easily, but it's fine here as the changes are rather minimal.

Thanks for your time to make sbt-native-packager more awesome 😎


/** Tag with version 'latest' */
val latest = s"$untagged:latest"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RIP binary API...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really time to add MiMa :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants