chore(integ-test-alpha): add log group retention days to waiter function

[sugardon]

Issue # (if applicable)

None
(related to #29277)

Reason for this change

When call waitForAssertions() the assertion provider creates CloudWatch log groups, but set to never expire

Description of changes

Added log retention days to assertion provider in WaiterStateMachine

Description of how you validated changes

Added unit test

Checklist

• My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

---

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

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[msambol]

for the maintainers to decide but do we actually want to set a reasonable default here? maybe 3 days or 1 week.

[TheRealAmazonKendra]

I think that no default is correct here. It's what was also done here #29277 (comment).

[msambol]

nit: could you remove the hyphen to match the other defaults ?

[TheRealAmazonKendra]

This is actually the correct way to specify this. When a descriptor is used instead of a value, the hyphen should be used.

[msambol]

👍🏼👍🏼

[TheRealAmazonKendra]

So, here's what I'm wondering overall: instead of a person having to specify this in each and every location we create these logs, could we just use where this is set on the assertions and then add the same here? Or, could we set this one the IntegTest construct and then use that value everywhere within the test infrastructure?

[sugardon]

Right now, I think people should specify log retention every location, like below.

import * as logs from 'aws-cdk-lib/aws-logs';

declare const testCase: IntegTest;
declare const start: IApiCall;

const describe = testCase.assertions.awsApiCall('StepFunctions', 'describeExecution', {
  executionArn: start.getAttString('executionArn'),
  // already we can set log retention after #29277
  RetentionDays: logs.RetentionDays.ONE_WEEK,
}).expect(ExpectedResult.objectLike({
  status: 'SUCCEEDED',
})).waitForAssertions({
  totalTimeout: Duration.minutes(5),
  interval: Duration.seconds(15),
  backoffRate: 3,
  // this PR
  logRetention: logs.RetentionDays.ONE_WEEK,
});

could we set this one the IntegTest construct and then use that value everywhere within the test infrastructure?

I think it's a good interface! But, I'm not sure how to implement it right now. If possible, I would like to implement it.

[comcalvi]

could we add this property to an existing integration test?

[sugardon]

Yes, we can add logRetetion to each existing assertion.
But, if we set logRetention everywhere in the Integ-test, this field will be pretty complex.

I think it would be more useful if we could set a default value, like below. (This idea is already mentioned at #29568 (comment).)

const testCase = new IntegTest(app, 'CustomizedDeploymentWorkflow', {
  testCases: [stackUnderTest],
  diffAssets: true,
  stackUpdateWorkflow: true,
  // Set log retention everywhere within the test infrastructure.
  // This values default logRetention for the test infrastructure.
  logRetention: logs.RetentionDays.ONE_WEEK,
});

[comcalvi]

We don't need to set it everywhere, but I agree that the interface you're proposing here is better anyway. The best way to implement this is likely with Aspects: https://docs.aws.amazon.com/cdk/v2/guide/aspects.html.

Create an Aspect on the IntegTest construct that checks if a node is an instance of the construct you want to configure logRetention on and set it to the value that was passed to the IntegTest's logRetention.

Something like:

class LogRetentionSetter implements IAspect {
    public visit(node: IConstruct): void {
     // alternatively, use a Symbol to identify the IntegTest classes you want, if instanceof doesn't work.
      if (node instanceof AssertionsProvider) { 
        // You might have to make the property lazy, or you might have to go down to the L1 itself and set the property there. 
        (node as any).logRetention = logs.RetentionDays.ONE_WEEK,
      }
    }
}

Aspects can be a bit tricky, so let me know what happens if you try this!

[sugardon]

Thank you for instructive comment!
I think, it might take time, but I will try it!

[comcalvi]

We don't need to set it everywhere, but I agree that the interface you're proposing here is better anyway. The best way to implement this is likely with Aspects: https://docs.aws.amazon.com/cdk/v2/guide/aspects.html.

Create an Aspect on the IntegTest construct that checks if a node is an instance of the construct you want to configure logRetention on and set it to the value that was passed to the IntegTest's logRetention.

Something like:

class LogRetentionSetter implements IAspect {
    public visit(node: IConstruct): void {
     // alternatively, use a Symbol to identify the IntegTest classes you want, if instanceof doesn't work.
      if (node instanceof AssertionsProvider) { 
        // You might have to make the property lazy, or you might have to go down to the L1 itself and set the property there. 
        (node as any).logRetention = logs.RetentionDays.ONE_WEEK,
      }
    }
}

Aspects can be a bit tricky, so let me know what happens if you try this!

[aws-cdk-automation]

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]

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.