fix: preserve original docker config if credentialhelpers exists #30750
Conversation
github-actions
bot
added
bug
gyalai-aws
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
|
Exemption Request |
aws-cdk-automation
added
the
pr-linter/exemption-requested
gyalai-aws
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
gyalai-aws
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
gyalai-aws
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
gyalai-aws
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
gyalai-aws
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
gyalai-aws
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
There was a problem hiding this comment.
As a heads up, we are currently doing work on re-designing the package structure of cdk-assets: #30691. This PR may need to be remade / reflected in the standalone library.
Thanks for this contribution! I think appending a local docker config to the credentials is an improvement.
My only concern is introducing errors where users previously did not encounter them.
Can you give some context on docker config files?
| if (e instanceof SyntaxError) { | ||
| throw new Error(`Unable to parse \'${this.dockerConfigFile()}\' in order to determine the configuration. Please ensure \'${this.dockerConfigFile()}\' is a valid JSON.`); | ||
| } | ||
| throw e; |
There was a problem hiding this comment.
Is this worth throwing an error for? Since this is new behavior, we should consider falling back to the old behavior on failures. We can still give a warning.
There was a problem hiding this comment.
Maybe this is not a big concern since it is only when the config file exists and is invalid. That would indicate the user wants to use the config file, but it is not written correctly. This case also seems rare.
Still something to think about for backwards compatability.
| test('returns false if no cred config exists', () => { | ||
| expect(docker.configureCdkCredentials()).toBeFalsy(); | ||
| }); |
There was a problem hiding this comment.
Was this expected behavior before, or is this new?
If not new, would this have been covered by one of the above tests - i.e. would this have resulted in a shell error?
|
The pull request linter fails with the following errors: PRs must pass status checks before we can provide a meaningful review. If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing ✅ A exemption request has been requested. Please wait for a maintainer's review. |
1 similar comment
|
The pull request linter fails with the following errors: PRs must pass status checks before we can provide a meaningful review. If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing ✅ A exemption request has been requested. Please wait for a maintainer's review. |
Issue # (if applicable)
Closes #30743.
Reason for this change
cdk-assets docker credential configuration overwrite the original docker configurations , etc proxy
Description of changes
Check whenever a ~./docker/config.json file exists and merge both files.
Description of how you validated changes
Unit tests are added
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license