feat(registry): add podman auth.json as an alternate location #897
Conversation
Adds podman default credential location as an alternate location when loading the config for containerd registry resolver Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
|
Hi @jpower432. Thanks for your PR. I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
openshift-ci
bot
added
the
needs-ok-to-test
|
cc @dinhxuanvu |
Codecov Report
@@ Coverage Diff @@
## master #897 +/- ##
=======================================
Coverage 52.07% 52.07%
=======================================
Files 103 103
Lines 9092 9092
=======================================
Hits 4735 4735
Misses 3449 3449
Partials 908 908 Continue to review full report at Codecov.
|
joelanford
left a comment
joelanford
left a comment
There was a problem hiding this comment.
Nice!
Just a few comments/questions.
| switch _, err := os.Stat(dockerConfigJSON); { | ||
| case err == nil: |
There was a problem hiding this comment.
Does it make sense to always attempt to read both files and then merge them?
There was a problem hiding this comment.
There is no function in the config library that I saw capable of merging, but I could write a helper function to merge the two configs. I definitely think that it would cover more use cases, but we would have to have a preferred option when merging. Assuming we would use the docker config as the preferred option?
There was a problem hiding this comment.
Yeah favoring the docker config option makes sense so we avoid breaking people that already have both and have different credentials for the same registry in the podman auth file.
There was a problem hiding this comment.
This is a nit, btw. If its hard to do, I don't consider it a must-have.
There was a problem hiding this comment.
Understood. I don't believe it would be overly complicated, but I would like to tackle it in a follow-on PR if that works.
|
/ok-to-test |
openshift-ci
bot
added
ok-to-test
…ross-platform option Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
Signed-off-by: Jennifer Power <barnabei.jennifer@gmail.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dinhxuanvu, jpower432 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 |
openshift-ci
bot
added
the
approved
|
@joelanford May you bless this with a lgtm? ;) |
|
/lgtm |
4 participants
Adds podman default credential location as an alternate location
when loading the config for registry resolver
Signed-off-by: Jennifer Power barnabei.jennifer@gmail.com
Description of the change:
This PR changes the
containerdregistry resolver to usedockerwell-known credential locations as a primary credential location for registry authentication and uses thepodmanlocation as an alternative.Motivation for the change:
The motivation for this change is to allow podman users to use the default location as a credential location and to help resolve an issue in
oc-mirrorReviewer Checklist
/docs