fix(cloudwatch): correct CompositeAlarm.fromCompositeAlarmName ARN format

[nakedible-p]

Fixes #24594.

[aws-cdk-automation]

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

[nakedible-p]

Exemption Request: Fix is in resource import, which isn't visible in stack output except if doign some other usage.

All in all, not sure what is the best way to test the fix, as there were no tests for the original functionality (which is obviously why it was broken in the first place).

Somebody more knowledgeable could give me a hint.

[aws-cdk-automation]

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.

[ayush987goyal]

@nakedible-p Can you mark this PR ready for review?

@pahud Can we create an exemption for the linter?

[aws-cdk-automation]

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]

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]

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.

[pahud]

Hi

Fixes must contain a change to an integration test file and the resulting snapshot.

You will need to submit an integration test file change and re-generate the snapshot.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

And I see some file conflicting you will need to resolve to pass the check before the maintainers can come in for initial review.

[nakedible-p]

Hi Pahud

Pull request now marked ready for review.

And as explained above, I don't know how to get the integration test thing fixed, as there are no integration tests for imports in this module.

Exemption Request: Fix is in resource import, which isn't visible in stack output except if doing some other usage.

All in all, not sure what is the best way to test the fix, as there were no tests for the original functionality (which is obviously why it was broken in the first place).

Somebody more knowledgeable could give me a hint.

The fix is trivial, but I do need assistance on how to write a test that didn't exist in the first place.

[ayush987goyal]

@nakedible-p You can refer the following examples:

1. DDB: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-dynamodb/test/dynamodb.test.ts#L2132
2. S3: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-s3/test/bucket.test.ts#L794

[nakedible-p]

Neither of those is an integration test (integ.*.ts) and the linter wants me to add an integration test.

[ayush987goyal]

@nakedible-p We should add this change for IAlarm as well to make it a consistent experience.

Regarding integration tests, you can refer this guide and an example here

On a high level it could look like what we have below where we create two stacks (one producer and one consumer)

import * as cw from 'aws-cdk-lib/aws-cloudwatch';
import * as cdk from 'aws-cdk-lib/core';
import { ExpectedResult, IntegTest } from '@aws-cdk/integ-tests-alpha';

const ALARM_NAME = "SomeAlarmName";
const COMPOSITE_ALARM_NAME = "SomeCompositeAlarmName";

class ProducerStack extends cdk.Stack {

  constructor(scope: cdk.App, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const alarm = new cw.Alarm(this, "SomeAlarm", {...});

    const compositeAlarm = new cw.CompositeAlarm(this, "SomeCompositeAlarm", {...});
  }
}

class ConsumerStack extends cdk.Stack {

  constructor(scope: cdk.App, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const importedAlarm = cw.Alarm.fromAlarmName(this, "ImportedSomeAlarm", ALARM_NAME);
    const importedCompositeAlarm = cw.CompositeAlarm.fromCompositeAlarmName(this, "ImportedSomeAlarm", COMPOSITE_ALARM_NAME);

    // create a new composite alarm from the two imported alarms above.
    const compositeAlarm = new cw.CompositeAlarm(this, "SomeCompositeAlarm", {...});
  }
}

const app = new cdk.App();
const producerStack = new ProducerStack(app, 'alarm-import-producer-integ');
const consumerStack = new ConsumerStack(app, 'alarm-import-consumer-integ');
consumerStack.addDependency(producerStack);

const testCase = new IntegTest(app, 'ImportAlarms', {
  testCases: [producerStack, consumerStack],
});

// Add assertions here that the new compositeAlarm in consumerStack exists.
testCase.assertions
  .awsApiCall('CloudWatch', 'DescribeAlarms', {...})
  .expect(ExpectedResult.objectLike(...));

[nakedible-p]

To clarify:

• I'm happy to spend the 15 minutes it takes to make an obvious fix to an obvious bug.
• I'm happy to spend the 30 minutes to write a simple regression test case that didn't exist for the feature.
• I'm happy to spend 30 minutes more to cross the t's and dot the i's to get a pull merged.

I'm not interested in:

• Spending hours figuring out where or how I need to run yarn integ when the docs do not really give a clue
• Spending hours ensuring the test suite passes in GitHub Codespaces when it mysteriously freezes mid-run
• Spending hours building a comprehensive test suite for a feature that had no tests at all
• Spending hours refactoring also Alarm to match the API of CompositeAlarm when they were inconsistent to start with

If you can help me in exactly what commands I need to run on a freshly checked out repository in order to run integration tests on packages/aws-cdk-lib/aws-cloudwatch, I can add a simple integration test which includes the composite alarm name and ARN in the generated stack, so the snapshot will catch any inadvertent changes. Otherwise, I'm happy to leave this work continued by someone else.

(Sorry, no coffee this morning yet)

[ayush987goyal]

I agree and that's why I requested @pahud to override that pr-rule for this change since I have typically not seen any service module having integ-tests for importing resources. I provided the above reference since you said you needed some assistance in writing them.

I am okay in not having integ-tests for this change and have appropriate coverage with just unit-tests. But I would really love to see the same API added to Alarm to be consistent even though they were not consistent to being with. Then again this is not something which should block this fix to be merged but a mere request.

@pahud What are you thoughts on this?

[nakedible-p]

Hold your horses - after some coffee, I found out the following:

• All the integration tests for aws-cdk-lib have been moved to @aws-cdk-testing/framework-integ
• There's plenty of existing integration tests for Cloudwatch
• I can probably manage to run the integration tests now that I found out from the build logs how they are run
• Integration test documentation is woefully out of date on these

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

[nakedible-p]

How about it now?

• Added missing method on Alarm
• Added tests on Alarm as well
• Added tests on the ARN variants of import on both of them (so we know ARN parsing works)
• Added integration test for composite alarm which ensures the alarmName and alarmArn stay stable by exposing them as outputs from the stack

Also, it seems that it is already known that the integration tests document is out of date: #24958

[ayush987goyal]

LGTM!
Thanks a lot for going the extra mile to add the functionality to Alarm and corresponding missing unit tests as well!

[nakedible-p]

Do I need to do something else to push this through, or will the AWS reviewers just pick this up when they have the time?

[corymhall]

Thanks for the PR!

[mergify]

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

[nakedible-p]

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

I think auto merge failed because of commit c174a49

Should I manually update the branch, or will someone else handle it from here?

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[mergify]

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