Skip to content
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

fix(eks): nested OCI repository names for private ECR helmchart deployments are not properly handled #23378

Conversation

psemeniuk
Copy link
Contributor

Fixes #22340

I've changed the way of extracting components (registry domain & region) from repository to regexp with named capture groups.

I've tested it by extending integ.eks-helm-asset.ts with second deployment from manually created private ECR. But I didn't provide coverage for this issue, because before deployment private ECR has to be created and filled by an example chart - i'm not aware of any simple way to automate a second part in tests.


All Submissions:

Adding new Construct Runtime Dependencies:

  • This PR adds new construct runtime dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Dec 16, 2022

@github-actions github-actions bot added bug This issue is a bug. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Dec 16, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team December 16, 2022 16:29
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@aws-cdk-automation
Copy link
Collaborator

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
Copy link
Collaborator

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 aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jan 15, 2023
@TheRealAmazonKendra
Copy link
Contributor

I've tested it by extending integ.eks-helm-asset.ts with second deployment from manually created private ECR. But I didn't provide coverage for this issue, because before deployment private ECR has to be created and filled by an example chart - i'm not aware of any simple way to automate a second part in tests.

Add the steps for verification in a test file so that we have documentation in code of how this was tested. Please also add in the comments here the output of those tests. In the meantime, I'm going to re-open this and add exemptions so it doesn't get closed again.

@TheRealAmazonKendra TheRealAmazonKendra added pr/do-not-merge This PR should not be merged at this time. pr-linter/exempt-test The PR linter will not require test changes pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. labels Jan 24, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review January 24, 2023 04:52

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@psemeniuk psemeniuk force-pushed the psemeniuk/handle_private_ecr_nested_repository_names branch from 423a296 to b7f4c34 Compare January 24, 2023 13:18
@psemeniuk
Copy link
Contributor Author

Manual testing steps added

Output:

$ integ-runner --update-on-failed integ.eks-helm-asset.js

Verifying integration test snapshots...

  CHANGED    integ.eks-helm-asset 5.066s
      Resources
[~] Custom::AWSCDK-EKS-HelmChart Clustercharttestocichart9C188967 
 └─ [~] Repository
     ├─ [-] oci://public.ecr.aws/aws-controllers-k8s/s3-chart
     └─ [+] oci://REDACTED.dkr.ecr.us-east-1.amazonaws.com/helm-charts-test/s3-chart



Snapshot Results: 

Tests:    1 failed, 1 total

Running integration tests for failed tests...

Running in parallel across regions: us-east-1, us-east-2, us-west-2
Running test /workspace/aws-cdk/packages/@aws-cdk/aws-eks/test/integ.eks-helm-asset.js in us-east-1
  SUCCESS    integ.eks-helm-asset-aws-cdk-eks-helm/DefaultTest 3179.091s
       NO ASSERTIONS

Test Results: 

Tests:    1 passed, 1 total
Done in 3185.22s.

@TheRealAmazonKendra TheRealAmazonKendra removed the pr/do-not-merge This PR should not be merged at this time. label Feb 10, 2023
@TheRealAmazonKendra TheRealAmazonKendra changed the title fix(eks): handle nested OCI repository names for private ECR helmchart deployment fix(eks): nested OCI repository names for private ECR helmchart deployments are not properly handled Feb 10, 2023
@mergify
Copy link
Contributor

mergify bot commented Feb 10, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review February 13, 2023 20:52

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Feb 13, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@TheRealAmazonKendra
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Feb 16, 2023

update

❌ Base branch update has failed

refusing to allow a GitHub App to create or update workflow .github/workflows/yarn-upgrade-v1main.yml without workflows permission
err-code: 0F0D0

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review February 16, 2023 06:09

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: c00d80b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Feb 16, 2023

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[aws-eks]: deploying helmcharts doesn't work when repository name has slash in ECR
3 participants