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

chore(cli-testing): add a retry for test #29871

Closed
wants to merge 4 commits into from

Conversation

TheRealAmazonKendra
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra commented Apr 17, 2024

One of our tests can remove customPermissionsBoundary creates a policy using createPolicy. Change to IAM policies/roles use eventual consistency. So, while the changes will show up right away if we were to call an API to describe that policy/role, the updates may not have actually propagated to all regions yet. This is likely the cause of the intermittent test failures for this test.

This change adds the eventually block and uses it to retry initial creation of this stack in the case that the policy changes have not made it to the relevant region just yet.

Checklist


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

One of our tests can remove customPermissionsBoundary creates a policy using createPolicy.
Change to IAM policies/roles use eventual consistency. So, while the changes will show up right
away if we were to call an API to describe that policy/role, the updates may not have actually propagated
to all regions yet. This is likely the cause of the intermittant test failures for this test.

This change adds the eventually block and uses it to retry initial creation of this stack in the case
that the policy changes have not made it to the relevant region just yet.
@aws-cdk-automation aws-cdk-automation requested a review from a team April 17, 2024 20:28
@github-actions github-actions bot added the p2 label Apr 17, 2024
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 17, 2024
@TheRealAmazonKendra
Copy link
Contributor Author

Linter isn't asking us to send this to the pipeline?

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Apr 17, 2024
@TheRealAmazonKendra
Copy link
Contributor Author

@Mergifyio update

Copy link
Contributor

mergify bot commented Apr 18, 2024

update

✅ Branch has been successfully updated

* @param fn function to run
* @param options EventuallyOptions
*/
const eventually = async <T>(call: () => Promise<T>, options?: EventuallyOptions): Promise<T> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if eventually is a standard naming convention but I love it.

param => (param.ParameterKey === 'InputPermissionsBoundary' && param.ParameterValue === policyName),
)).toEqual(true);
// Policy creation and consistency across regions is "almost immediate"
// See: https://docs.aws.amazon.com/IAM/latest/UserGuide/troubleshoot_general.html#troubleshoot_general_eventual-consistency
Copy link
Contributor

Choose a reason for hiding this comment

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

Great catch on this and thank you for also adding it as a comment!

)).toEqual(true);
};

await eventually(createStackWithPermissionBoundary, { maxAttempts: 3 });
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking on this, but just curious why 3?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Arbitrarily seemed reasonable.

Copy link
Contributor

@colifran colifran left a comment

Choose a reason for hiding this comment

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

LGTM!

@aws-cdk-automation
Copy link
Collaborator

@Mergifyio update

Copy link
Contributor

mergify bot commented Apr 19, 2024

update

✅ Branch has been successfully updated

@aws-cdk-automation aws-cdk-automation added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Apr 19, 2024
@aws-cdk-automation
Copy link
Collaborator

@Mergifyio update

Copy link
Contributor

mergify bot commented Apr 19, 2024

update

✅ Branch has been successfully updated

@aws-cdk-automation aws-cdk-automation added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested labels Apr 19, 2024
@aws-cdk-automation aws-cdk-automation added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Apr 19, 2024
@aws-cdk-automation
Copy link
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants