-
Notifications
You must be signed in to change notification settings - Fork 158
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
Support tag task name for 'tags' #205
Conversation
Thanks for your interest in palantir/gradle-docker, @higankanshi! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
Any update here? @pkoenig10 |
@uschi2000 @pkoenig10 |
Hej @higankanshi , sorry this slipped through the cracks. I'll take a look now. |
@uschi2000 Thanks for your time. |
Thinking about this change a bit, I wonder if we should take a harder break and rectify some of the config birth defects, in particular the asymmetry between image
My rationale is that while your proposed is clearly something we should do (in order to fix all those issues), the implementation makes the config language pretty complicated (as witnessed by constraints like Thoughts? |
@uschi2000 docker tag httpd fedora/httpd:version1.0
docker tag httpd:test fedora/httpd:version1.0.test So, i think that tag can be include a repo host, new image name. docker {
tag 'repo:port/foo:1.0' // Invalid tag, because we can get a valid name for this tag
tag 'foo:latest' // Maybe can make a dockerFooLatest task ?
tag 'latest' // Maybe we should not support version only for defined a tag
tag 'otherRepo/foo:latest', myTaskName // Good, a custom name
} But if we do this, it would be a breaking change for this plugin... Using @ for backward compatible |
I agree with @uschi2000. It's seems odd to me that I think we still need the Providing custom task names seems like a step in the right direction, since it seems hopeless for us to try and derive task names if given tags which may or may not contain a registry, repository, image name, and/or tag. |
@pkoenig10 How do you think about using @ for marking task name? |
@pkoenig10 if you want to do the DSL break, then we'd also have to think about excavating our internal repos to get them towards the new config. should be relatively simple, but just flagging. |
Ideally I'd like to avoid a DSL break here. Using @ for marking task name seems like a reasonable approach. This should be safe since @ cannot be used in tag names. It looks like the primary blocker to supporting the better DSL is the Eventually it would be good to push everyone to the better DSL. This probably will take a couple of steps:
And then optionally, to remove custom syntax from this plugin: |
OK, got it, I will change my PR later |
@uschi2000 @pkoenig10 docker {
tag 'newTag', 'repo:port/foo:1.0'
} However, it doesn't resolve image name for the tag. If you want to retag the version, you must specify the image name too. docker {
tag 'latest', 'latest' // Invalid tag, it must be "tag 'latest', 'name: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.
Design looks good, couple of implementation comments
src/test/groovy/com/palantir/gradle/docker/PalantirDockerPluginTests.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/com/palantir/gradle/docker/DockerExtension.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/com/palantir/gradle/docker/DockerExtension.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/com/palantir/gradle/docker/DockerExtension.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/com/palantir/gradle/docker/PalantirDockerPlugin.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/com/palantir/gradle/docker/PalantirDockerPlugin.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/com/palantir/gradle/docker/PalantirDockerPlugin.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/com/palantir/gradle/docker/PalantirDockerPlugin.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/com/palantir/gradle/docker/PalantirDockerPlugin.groovy
Outdated
Show resolved
Hide resolved
c07a471
to
cfa1c4e
Compare
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, couple of minor comments
src/main/groovy/com/palantir/gradle/docker/PalantirDockerPlugin.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/com/palantir/gradle/docker/PalantirDockerPlugin.groovy
Outdated
Show resolved
Hide resolved
Fixed |
Description
This PR make 'tags' more powerful, you can use new 'tag' DSL to give the tag a task name.
There are some samples:
It will generate tag tasks with name
dockerTagWithTaskName
anddockerTagNewImageName
, and push tasks with namedockerPushWithTaskName
anddockerPushNewImageName
.With task name, we can build some diff name image of one image, push to diff repo host.
Note
'tags' DSL can resolve the tag name auto, but 'tag' can not. It means you must specify the image name for 'tag' DSL.
There are some sample:
Tag with just new version:
name:
old-name:test
tags dsl:
tags 'latest'
tag dsl:
tag 'latest', 'docker-name:latest'
image
result name: docker-name:latest
Tag with new name:
name:
old-name:test
tags dsl:
tags 'newName@docker-name:latest'
tag dsl:
tag 'newName', 'docker-name:latest'
image
result name: docker-name:latest
Tag with new repo host:
name:
old-name:test
tag:
newName@host:port/repo/docker-name:latest
tags:
newName', 'host:port/repo/docker-name:latest'
image result name:
host:port/repo/docker-name:latest
New tasks
dockerTagsPush
Pushes all tagged Docker images to configured Docker Hub.
dockerAllPush
Pushes all tagged Docker images and named Docker image to configured Docker Hub.
Solved issues
#195 Allow dockerPush to target multiple repositories
Use
dockerPush<Tag>
to diff repositoriesRun this command:
#181 Pushing just the defined tags
Use
dockerTagsPush
task can solve this issueRun this command:
#95 Make push task depend on push tasks for each tag if tags are present
Use
dockerTagsPush
anddockerPush
can solve this issue/PR without change behavior ofdockerPush
Run this command:
#149 dockerPush doesn't generate (and thus push) all Tags
#94 Ability to easily push all tags for an image
#209 Docker tag should support full retagging
Test sample
There are some sample which used in test: