aws-cdk
aws-cdk copied to clipboard
fix(cdk-assets): Remove https from endpoint, to work with podman
This PR is a redo of https://github.com/aws/aws-cdk/pull/19115
podman does not allow prefixing the endpoint with https when calling docker login:
fail: docker login --username AWS --password-stdin https://XXX.dkr.ecr.eu-west-1.amazonaws.com exited with error code 125: Error: credentials key has https[s]:// prefix
This PR removes the prefix, which works with both docker and podman
Closes #16209
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
As an aside, I was not able to run the unit tests locally, the would fail with Cannot read asset manifest at '/simple/cdk.out': ENOENT: no such file or directory, stat '/simple/cdk.out', seeming to suggest that mock-fs is not working as expected. However, I removed the https prefix from all existing tests, and will cross my fingers and see what the CI says.
Exemption Request: I do not think this change qualifies for an integration test. It should not change the output of cdk synth.
Additionally, the PR already changes unit tests, which should be enough to catch future regressions
Turns out mock-fs does not work with node 20 https://github.com/tschaub/mock-fs/issues/384
Managed to fix the latest failing test :crossed_fingers:
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.
@aws-cdk-automation Can I talk to a human please? I believe my PR is ready for review
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.
@aws-cdk-automation Can I talk to a human please? I believe my PR is ready for review
Hello, you've reached a CDK Human. We cannot come to the PR right now so please leave a... just kidding.
Apologies for the delay on this review!
I don't think that we can just remove the https across the board as it might cause issues elsewhere. We do have an existing integ test for deploying with a docker asset so I'm going to start with running this through our test pipeline.
@Mergifyio update
update
❌ Mergify doesn't have permission to update
For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/request-cli-integ-test.yml without workflows permission
@aws-cdk-automation Can I talk to a human please? I believe my PR is ready for review
Hello, you've reached a CDK Human. We cannot come to the PR right now so please leave a... just kidding.
Apologies for the delay on this review!
No worries, I've been able to patch my local installation in the meantime, but looking forward to getting this merged :man_dancing:
I don't think that we can just remove the
httpsacross the board as it might cause issues elsewhere. We do have an existing integ test for deploying with a docker asset so I'm going to start with running this through our test pipeline.
According to a comment in the origin issue, docker should be able to run without https https://github.com/aws/aws-cdk/issues/16209#issuecomment-913494909. I'm hoping the integration tests agree :crossed_fingers:
Would be nice if this can be merged. I currently cannot build assets within CDK.
@TheRealAmazonKendra Friendly ping - Is there anything I can do to bring this over the finish line?
@Mergifyio update
update
❌ Mergify doesn't have permission to update
For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/github-merit-badger.yml without workflows permission
Sorry for the radio silence on this one. We are moving cdk-assets out of the main repo and into a repo of it's own, which is still in progress. As long as the test run I'm doing now works, I'll happily move this over as well but I wanted to get your preference on how we go about that. I can move it for you so you don't have to go to the extra effort, but that removes the credit to you in GitHub for the change. Alternatively, you can open a new PR in the new repo, but that's obviously extra work on your end.
Any preference here?
:arrow_right: PR build request submitted to test-main-pipeline :arrow_left:
A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.
AWS CodeBuild CI Report
- CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
- Commit ID: e958e5e7ff24fee15443b028dedf6ab9990ad545
- Result: SUCCEEDED
- Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
Sorry for the radio silence on this one. We are moving cdk-assets out of the main repo and into a repo of it's own, which is still in progress. As long as the test run I'm doing now works, I'll happily move this over as well but I wanted to get your preference on how we go about that. I can move it for you so you don't have to go to the extra effort, but that removes the credit to you in GitHub for the change. Alternatively, you can open a new PR in the new repo, but that's obviously extra work on your end.
Any preference here?
Point me to the new repo, and I'll make a PR :)