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.

Sign up for GitHub

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

Jump to bottom

fix(eks): fix helm deploy login for public ECR repositories #24104

Merged
merged 32 commits into from
Feb 23, 2023
Merged

fix(eks): fix helm deploy login for public ECR repositories #24104

merged 32 commits into from
Feb 23, 2023

Conversation

zorrofox
Copy link
Contributor

@zorrofox zorrofox commented Feb 10, 2023 •
edited
Loading

fix helm deploy login for public ECR repositories

I have tested this issue fixed in us-east-1 and us-west-2 integ testing for

yarn integ-runner integ.eks-helm-asset.js --force --parallel-regions us-east-1
yarn integ-runner integ.eks-helm-asset.js --force --parallel-regions us-west-2

Closes #23977.

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 Feb 10, 2023

@aws-cdk-automation aws-cdk-automation requested a review from a team February 10, 2023 10:45
@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Feb 10, 2023
aws-cdk-automation
aws-cdk-automation previously requested changes Feb 10, 2023
View reviewed changes
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.

TheRealAmazonKendra
TheRealAmazonKendra previously requested changes Feb 12, 2023
View reviewed changes
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! This is close. See: #23176.

@TheRealAmazonKendra TheRealAmazonKendra mentioned this pull request Feb 12, 2023
Closed
@zorrofox
Copy link
Contributor Author

Do I need create new Integ testing for this? I know there already have a pull request for this bug. But #23176 not be merged.

@TheRealAmazonKendra
Copy link
Contributor

#23176

No need, the integ test covers this. Any test that would prove a failure would fail the test as well. Adding an exemption.

@TheRealAmazonKendra TheRealAmazonKendra added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Feb 13, 2023
@TheRealAmazonKendra
Copy link
Contributor

I more meant to point to the need to add the role added in that PR.

@aws-cdk-automation aws-cdk-automation dismissed their stale review February 13, 2023 17:32

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

@otterley
Copy link
Contributor

I'd like to request this be merged ASAP. Currently no version of Karpenter newer than 0.17 (which is rather old now) can be installed by a CDK app because of the issue being fixed by this PR.

@TheRealAmazonKendra
Copy link
Contributor

I'd like to request this be merged ASAP. Currently no version of Karpenter newer than 0.17 (which is rather old now) can be installed by a CDK app because of the issue being fixed by this PR.

It still requires the role to be added. We will review it once it's been completed and marked as ready.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review February 14, 2023 02:35

Pull request has been modified.

@TheRealAmazonKendra
Copy link
Contributor

The integ tests will need updating. If you need assistance running them, please let me know.

@zorrofox
Copy link
Contributor Author

@TheRealAmazonKendra the commits need rerun 9 integ testings and 7 have passed, but integ.alb-controller and integ.eks-service-account-sdk-call run into failed because Pinger test error. But I don't think these issues are caused by these modifications and both integ testings will run into errors even no these commits. Can you clarify this?

@otterley
Copy link
Contributor

@zorrofox @TheRealAmazonKendra These two tests have proved to be extremely flaky and racy. @pahud and I have had to re-run them multiple times to get a passing result.

I believe you can merge this request if these are the only two tests that are failing.

@zorrofox
Copy link
Contributor Author

I will mark the PR as ready for review! Thanks, @otterley .

@zorrofox zorrofox marked this pull request as ready for review February 16, 2023 01:05
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

2 similar comments
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@mergify
Copy link
Contributor

mergify bot commented Feb 23, 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).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 71ec6b6 into aws:main Feb 23, 2023
@mergify
Copy link
Contributor

mergify bot commented Feb 23, 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).

Naumel pushed a commit that referenced this pull request Feb 24, 2023
@zorrofox @Naumel
fix(eks): fix helm deploy login for public ECR repositories (#24104)
0119e8e 
fix helm deploy login for public ECR repositories

I have tested this issue fixed in `us-east-1` and `us-west-2` integ testing for
```
yarn integ-runner integ.eks-helm-asset.js --force --parallel-regions us-east-1
yarn integ-runner integ.eks-helm-asset.js --force --parallel-regions us-west-2
```

Closes #23977.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
beck3905 pushed a commit to beck3905/aws-cdk that referenced this pull request Feb 28, 2023
@zorrofox @beck3905
fix(eks): fix helm deploy login for public ECR repositories (aws#24104)
e06275b 
fix helm deploy login for public ECR repositories

I have tested this issue fixed in `us-east-1` and `us-west-2` integ testing for
```
yarn integ-runner integ.eks-helm-asset.js --force --parallel-regions us-east-1
yarn integ-runner integ.eks-helm-asset.js --force --parallel-regions us-west-2
```

Closes aws#23977.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@pahud pahud mentioned this pull request Mar 7, 2023
Closed
homakk pushed a commit to homakk/aws-cdk that referenced this pull request Mar 13, 2023
@zorrofox @homakk
fix(eks): fix helm deploy login for public ECR repositories (aws#24104)
7c0efba 
fix helm deploy login for public ECR repositories

I have tested this issue fixed in `us-east-1` and `us-west-2` integ testing for
```
yarn integ-runner integ.eks-helm-asset.js --force --parallel-regions us-east-1
yarn integ-runner integ.eks-helm-asset.js --force --parallel-regions us-west-2
```

Closes aws#23977.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@wongma7 wongma7 mentioned this pull request Mar 27, 2023
Closed
homakk pushed a commit to homakk/aws-cdk that referenced this pull request Mar 28, 2023
@zorrofox @homakk
fix(eks): fix helm deploy login for public ECR repositories (aws#24104)
735a828 
fix helm deploy login for public ECR repositories

I have tested this issue fixed in `us-east-1` and `us-west-2` integ testing for
```
yarn integ-runner integ.eks-helm-asset.js --force --parallel-regions us-east-1
yarn integ-runner integ.eks-helm-asset.js --force --parallel-regions us-west-2
```

Closes aws#23977.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@filletofish filletofish mentioned this pull request Jul 2, 2023
Merged
This was referenced Sep 13, 2024
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TheRealAmazonKendra TheRealAmazonKendra TheRealAmazonKendra approved these changes

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

Assignees
No one assigned
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

aws-eks: OCI HelmChart from public.ecr.aws only works in us-east-1
5 participants
@zorrofox @TheRealAmazonKendra @otterley @aws-cdk-automation @pahud