-
Notifications
You must be signed in to change notification settings - Fork 18.7k
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
Add ability to add multiple tags with docker build #15780
Conversation
I don't see a good reason to |
I'm +1, and it was already approved on in #863 (comment), but a very long time ago 😄 |
@@ -50,7 +50,7 @@ var validCommitCommands = map[string]bool{ | |||
type Config struct { | |||
DockerfileName string | |||
RemoteURL string | |||
RepoName string | |||
RepoName interface{} |
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.
We don't want to use an interface{}
here.
Entrpoiny/Cmd have some special parsing you can look at (in runconfig/config.go) to handle this.
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.
Cool! I think we can abstract the parser in runconfig to "opts" package so that we can reuse the code here.
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.
e0a9033
to
b206b2c
Compare
b206b2c
to
d0cbfa3
Compare
Test added. The failure of the experimental test is not caused by this PR. |
d0cbfa3
to
0f94106
Compare
@@ -40,7 +40,7 @@ var ( | |||
func() bool { | |||
// Set a timeout on the GET at 15s | |||
var timeout = time.Duration(15 * time.Second) | |||
var url = "https://hub.docker.com" | |||
var url = "http://www.baidu.com" |
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.
not sure this should be changed? (also this changes from a https:// to a http:// domain)
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.
Ah, sorry. It's quite slow to connect to docker hub in China so I changed the url temporarily but forgot to change it back before pushing to github.
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.
Heh, I thought that would probably be the case; given that we have quite some contributors from China; do you think we should have some alternatives?
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 it's okay to leave it as it is. But I have to be more careful if I change it again 😄
If in the future other contributors run into the same problem as mine, I think we can allow the user to pass the URL to check from an environment variable when running the tests.
0f94106
to
2195827
Compare
@mountkin I see you updated the PR; is this ready for another review? |
@thaJeztah I think it's ready for code review now. |
@mountkin what's the usecase? I don't feel strongly either way, don't mind if it gets in, don't mind if it doesn't. |
@tiborvass it's useful when I want to build an image and push it to multiple registries. Without this feature I have to run "docker tag" after the build. |
Fair enough, weak +1 for design |
It looks useful. Let's move it forward. |
@mountkin do you mind rebase and making sure we have proper tests and documentation for this? |
f046fe5
to
edc2518
Compare
@calavera rebased and updated docs. Thanks for helping to move this forward. |
LGMT |
Thanks @mountkin, docs LGTM, but unfortunately this needs a rebase 😢 ping @moxiegirl PTAL |
37f000a
to
fc42040
Compare
@thaJeztah rebased 😃 |
Thanks @mountkin ! ping @jamtur01 @moxiegirl @SvenDowideit ptal |
the resulting image in case of success. | ||
- **t** – A name and optional tag to apply to the image in the `name:tag` format. | ||
If you omit the `tag` the default `latest` value is assumed. | ||
You can provide one or more **t** parameters. |
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.
needs ticks not bold around the t so t
fc42040
to
6c86ab3
Compare
Signed-off-by: Shijiang Wei <mountkin@gmail.com>
6c86ab3
to
c2eb37f
Compare
Updated the docs per @moxiegirl's suggestions. |
Thanks @mountkin, docs LGTM! ping @SvenDowideit @moxiegirl PTAL |
LGTM! Thank you @mountkin. |
yay 🎉 |
Al green! Merging. Thanks @mountkin ! |
Add ability to add multiple tags with docker build
What about pushing multiple tags in one command as well? |
@liyaka there are still some outstanding issues / improvements to be made to push/pull, it has been decided to uphold new features in that until those improvements are made. Having said that; yes it would be nice to have :-) |
@thaJeztah Any news on multiple tags in single push? 😄 |
@SimenB We want to reserve the syntax in |
Closes #863
docker build -t tag1 -t tag2:v1 -t tagN .
TODO:
Signed-off-by: Shijiang Wei mountkin@gmail.com