-
Notifications
You must be signed in to change notification settings - Fork 43
Fix the name of the image in the github action #79
Conversation
Signed-off-by: Chuck Ha <chuckh@vmware.com>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chuckha The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@@ -11,7 +11,7 @@ action "Docker Registry" { | |||
action "build" { | |||
uses = "actions/docker/cli@master" | |||
needs = ["Docker Registry"] | |||
args = "build -t docker.pkg.github.com/kubernetes-sigs/cluster-api-provider-docker:latest ." | |||
args = "build -t docker.pkg.github.com/kubernetes-sigs/cluster-api-provider-docker/manager: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.
args = "build -t docker.pkg.github.com/kubernetes-sigs/cluster-api-provider-docker/manager:latest ." | |
args = "build -t docker.pkg.github.com/kubernetes-sigs/cluster-api-provider-docker/capd-manager: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.
that's already in the repository name, do you think it's unclear as is?
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 clear and would leave the image name as manager
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 i see your note about consistency in slack.
I do not see the consistency between CAPA/CAPI and I think this is a chance to create patterns that are less redundant than what already exist
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.
The image is called capd-manager
in the publish-manager.sh
Proposing to be consistent w/ the name
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 makes sense in publish-manager to keep it as capd-manager because gcr.io repositories are structured as gcr.io/<project-name>/<image-name>:<tag>
. Unfortunately the project-name is almost never going to be cluster-api-provider-docker related and thus the prefix capd- makes sense in that context.
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 am cool w/ that!
@@ -23,5 +23,5 @@ action "master" { | |||
action "push" { | |||
uses = "actions/docker/cli@master" | |||
needs = ["master"] | |||
args = "push docker.pkg.github.com/kubernetes-sigs/cluster-api-provider-docker:latest" | |||
args = "push docker.pkg.github.com/kubernetes-sigs/cluster-api-provider-docker/manager: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.
args = "push docker.pkg.github.com/kubernetes-sigs/cluster-api-provider-docker/manager:latest" | |
args = "push docker.pkg.github.com/kubernetes-sigs/cluster-api-provider-docker/capd-manager:latest" |
/lgtm |
/hold for @ncdc didn't know if he had comments |
All good here (also you have to put the slash command on its own line...) |
looks like the hold was too slow. :( |
/test pull-cluster-api-provider-docker-verify |
Signed-off-by: Chuck Ha chuckh@vmware.com
What this PR does / why we need it:
This PR attempts to fix the broken name on the docker push.
Special notes for your reviewer:
Release note: