Allow config.json, .dockerconfigjson in buildah ClusterTask #1078
Conversation
tekton-robot
added
the
release-note
tekton-robot
added
the
size/S
|
/cherrypick release-v0.60.x |
|
@concaf: once the present PR merges, I will cherry-pick it on top of release-v0.60.x in a new PR and assign it to you. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
This commit allows both, config.json and .dockerconfigjson files in the buildah ClusterTask with first priority for config.json. This is done since it's common practice to create and mount secrets with docker config.json in Kubernetes with the key as .dockerconfigjson which ends up being the file name.
concaf
force-pushed
the
buildah-dockerconfigjson
branch
from
927be4a
to
b58cc93
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester 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 |
tekton-robot
added
the
approved
| cp "$(workspaces.dockerconfig.path)/.dockerconfigjson" "$HOME/.docker/config.json" | ||
| export DOCKER_CONFIG="$HOME/.docker" | ||
|
|
||
| # need to error out if neither files are present |
There was a problem hiding this comment.
should we error if nothing is present?
There was a problem hiding this comment.
I think so. A secret was passed (otherwise we wouldn't go into this condition), but without any valid "data field", so it's probably better to fail "loudly and fast" than silently 🙃
There was a problem hiding this comment.
well, this is inside
if [[ "$(workspaces.dockerconfig.bound)" == "true" ]];
so the user has supplied a workspace that contains their docker config we couldn't find it, so i think we should be erroring out
There was a problem hiding this comment.
ah, vincent already replied :P
|
|
||
| # else we look for .dockerconfigjson at the root | ||
| elif test -f "$(workspaces.dockerconfig.path)/.dockerconfigjson"; then | ||
| cp "$(workspaces.dockerconfig.path)/.dockerconfigjson" "$HOME/.docker/config.json" |
There was a problem hiding this comment.
why do we need to copy, can't we just set this path to DOCKER_CONFIG
There was a problem hiding this comment.
Because the name is not correct 🙃 DOCKER_CONFIG is the path of a folder where docker (or buildah in that case) will look for a config.json file. The file in the workspace is call .dockerconfigjson 🙃
There was a problem hiding this comment.
DOCKER_CONFIG takes a directory which contains a file called config.json which is not the case here; we could copy (rename the file) in the workspace itself but we might not have write permissions there; so i chose to copy it to HOME
There was a problem hiding this comment.
ah, vincent already replied :P
|
/lgtm |
|
@concaf: new pull request created: #1079 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Changes
This commit allows both, config.json and .dockerconfigjson files in the buildah ClusterTask with first priority for config.json. This is done since it's common practice to create and mount secrets with docker config.json in Kubernetes with the key as .dockerconfigjson which ends up being the file name.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
make test lintbefore submitting a PRSee the contribution guide for more details.
Release Notes