aws-cdk icon indicating copy to clipboard operation
aws-cdk copied to clipboard

fix(cdk-assets): Remove https from endpoint, to work with podman

Open janmeier opened this issue 1 year ago • 6 comments

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.

janmeier avatar Jan 30 '24 13:01 janmeier

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

janmeier avatar Jan 30 '24 13:01 janmeier

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:

janmeier avatar Jan 30 '24 14:01 janmeier

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 avatar Feb 21 '24 00:02 aws-cdk-automation

@aws-cdk-automation Can I talk to a human please? I believe my PR is ready for review

janmeier avatar Feb 21 '24 19:02 janmeier

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 avatar Feb 28 '24 18:02 aws-cdk-automation

@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!

TheRealAmazonKendra avatar Apr 11 '24 00:04 TheRealAmazonKendra

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.

TheRealAmazonKendra avatar Apr 11 '24 00:04 TheRealAmazonKendra

@Mergifyio update

TheRealAmazonKendra avatar Apr 11 '24 01:04 TheRealAmazonKendra

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

mergify[bot] avatar Apr 11 '24 01:04 mergify[bot]

@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:

janmeier avatar Apr 11 '24 11:04 janmeier

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.

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:

janmeier avatar Apr 11 '24 11:04 janmeier

Would be nice if this can be merged. I currently cannot build assets within CDK.

araker avatar May 14 '24 13:05 araker

@TheRealAmazonKendra Friendly ping - Is there anything I can do to bring this over the finish line?

janmeier avatar Jul 18 '24 12:07 janmeier

@Mergifyio update

TheRealAmazonKendra avatar Jul 31 '24 17:07 TheRealAmazonKendra

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

mergify[bot] avatar Jul 31 '24 17:07 mergify[bot]

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?

TheRealAmazonKendra avatar Jul 31 '24 17:07 TheRealAmazonKendra

: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-cdk-automation avatar Jul 31 '24 17:07 aws-cdk-automation

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

aws-cdk-automation avatar Jul 31 '24 17:07 aws-cdk-automation

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 :)

janmeier avatar Aug 01 '24 08:08 janmeier