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(config): explicit ruleScope value overwritten on CloudFormationStackDriftDetectionCheck #27693

Closed

Conversation

sHtev
Copy link

@sHtev sHtev commented Oct 26, 2023

If ruleScope is set in the props of CloudFormationStackDriftDetectionCheck, and ownStackOnly is not set then the current code:

    this.ruleScope = RuleScope.fromResource( ResourceType.CLOUDFORMATION_STACK, props.ownStackOnly ? Stack.of(this).stackId : undefined );

overwrites this.ruleScope with a resource RuleScope with an undefined resource id.

This makes it impossible to scope the drift check based on tags or resource ids other than that of the current stack.

The change in this PR explicitly checks for the ownStackOnly prop before overriding the ruleScope, and sets a sensible default if ruleScope itself is undefined to mirror existing default behaviour.

An example with a tag-filtering RuleScope has been added to the unit tests.


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

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Oct 26, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team October 26, 2023 14:56
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.

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

@aws-cdk-automation aws-cdk-automation dismissed their stale review October 26, 2023 15:21

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

@sHtev
Copy link
Author

sHtev commented Oct 26, 2023

Note: the integration test currently has a warning it is undeployable, I hope this does not stop this needed fix being applied

@sHtev sHtev changed the title fix(config): don't overwrite existing ruleScope on CloudFormationStackDriftDetectionCheck fix(config): explicit ruleScope value overwritten on CloudFormationStackDriftDetectionCheck Oct 27, 2023
@sHtev
Copy link
Author

sHtev commented Oct 30, 2023

FWIW the integration test does work for me locally (node16, node18 and node20)

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the BUILD FAILING 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.

@sHtev
Copy link
Author

sHtev commented Nov 21, 2023

Exemption Request if someone could help me understand why the codebuild fails when integration works locally that would be great

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Nov 21, 2023
@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 Nov 26, 2023
@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.

@paulhcsun paulhcsun added pr-linter/do-not-close The PR linter will not close this PR while this label is present and removed closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. labels Dec 7, 2023
@paulhcsun paulhcsun reopened this Dec 7, 2023
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@paulhcsun paulhcsun changed the title fix(config): explicit ruleScope value overwritten on CloudFormationStackDriftDetectionCheck fix(config): explicit ruleScope value overwritten on CloudFormationStackDriftDetectionCheck Jan 29, 2024
@paulhcsun
Copy link
Contributor

Hi @sHtev, apologies for not getting around to this, if you're still interested in working on this I've taken a look at the codebuild failure, usually integ test failures in the PR pipeline are because the snapshots you've added don't match up with what's being created by the pipeline. Could you just double check that the snapshots included in the PR are the most recent snapshots you have from running the integ tests locally?

@paulhcsun
Copy link
Contributor

Closing this PR for staleness. Please open a new PR if you'd like to continue working on this @sHtev.

@paulhcsun paulhcsun closed this Feb 22, 2024
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 p2 pr-linter/do-not-close The PR linter will not close this PR while this label is present pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants