-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Correct ECR credential provider example #40841
Conversation
Welcome @cartermckinnon! |
/approve thanks @cartermckinnon |
LGTM label has been added. Git tree hash: 690d9877f44c38f6e77666ef02b46649ae3a49d0
|
deploy/netlify timed out after an hour 🤷 /retest |
@cartermckinnon: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
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.
Thanks.
I have a question about the region setting.
/sig cloud-provider
The configuration file passed into `--image-credential-provider-config` is read by the kubelet to determine which exec plugins | ||
should be invoked for which container images. Here's an example configuration file you may end up using if you are using the | ||
[ECR](https://aws.amazon.com/ecr/)-based plugin: | ||
[ECR-based plugin](https://github.com/kubernetes/cloud-provider-aws/tree/master/cmd/ecr-credential-provider): |
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.
Even better, if feasible, is to link to the AWS documentation for configuring this.
Do we know if there's anything to link to?
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 mkdocs don't have anything useful: https://cloud-provider-aws.sigs.k8s.io/#aws-credential-provider
My call was: it's better to link to the cred provider implementation versus some other doc
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.
https://cloud-provider-aws.sigs.k8s.io/#aws-credential-provider is actually Kubernetes documentation, not AWS documentation.
Sounds like there's no AWS docs on this detail.
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
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 for docs
The configuration file passed into `--image-credential-provider-config` is read by the kubelet to determine which exec plugins | ||
should be invoked for which container images. Here's an example configuration file you may end up using if you are using the | ||
[ECR](https://aws.amazon.com/ecr/)-based plugin: | ||
[ECR-based plugin](https://github.com/kubernetes/cloud-provider-aws/tree/master/cmd/ecr-credential-provider): |
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.
https://cloud-provider-aws.sigs.k8s.io/#aws-credential-provider is actually Kubernetes documentation, not AWS documentation.
Sounds like there's no AWS docs on this detail.
With #40841 (comment) in mind |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, sftim 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 |
/retest |
@cartermckinnon: Cannot trigger testing until a trusted user reviews the PR and leaves an 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. |
@sftim can you add ok-to-test so I can get this mergeable? |
The example credential provider config file is misleading in a few ways:
args
section implied thatget-credentials
is used by the ECR credential provider. It isn't.If this example is not intended to be for the
ecr-credential-provider
implemented inkubernetes/cloud-provider-aws
-- the second two points are arguable; but the first is not in my opinion.