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(secretsmanager): RotateSchedule is not disabled when automaticallyAfter is 0 #27497

Closed

Conversation

go-to-k
Copy link
Contributor

@go-to-k go-to-k commented Oct 11, 2023

This PR fixes the bug that a RotateSchedule is not disabled when automaticallyAfter is 0.

Closes #27460.


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-cdk-automation requested a review from a team October 11, 2023 15:49
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p2 admired-contributor [Pilot] contributed between 13-24 PRs to the CDK labels Oct 11, 2023
@go-to-k go-to-k changed the title fix(secretsmanager): automaticallyAfter is not disabling the automatic rotation of secrets fix(secretsmanager): RotateSchedule is not disabling the automatic rotation of secrets when automaticallyAfter is 0 Oct 11, 2023
@go-to-k go-to-k changed the title fix(secretsmanager): RotateSchedule is not disabling the automatic rotation of secrets when automaticallyAfter is 0 fix(secretsmanager): RotateSchedule is not disabling when automaticallyAfter is 0 Oct 11, 2023
@go-to-k go-to-k changed the title fix(secretsmanager): RotateSchedule is not disabling when automaticallyAfter is 0 fix(secretsmanager): RotateSchedule is not disabled when automaticallyAfter is 0 Oct 11, 2023
@go-to-k
Copy link
Contributor Author

go-to-k commented Oct 11, 2023

Duration.days(0) should disable automatic rotation.

  /**
   * Specifies the number of days after the previous rotation before
   * Secrets Manager triggers the next automatic rotation.
   *
   * A value of zero will disable automatic rotation - `Duration.days(0)`.
   *
   * @default Duration.days(30)
   */
  readonly automaticallyAfter?: Duration;

/**
* Specifies the number of days after the previous rotation before
* Secrets Manager triggers the next automatic rotation.
*
* A value of zero will disable automatic rotation - `Duration.days(0)`.
*
* @default Duration.days(30)
*/
readonly automaticallyAfter?: Duration;

@go-to-k go-to-k marked this pull request as ready for review October 11, 2023 16:48
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 11, 2023
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks 👍

@aws-cdk-automation aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Oct 12, 2023
@scanlonp
Copy link
Contributor

I believe that the current behavior is intended (see discussion on issue #27460 (comment)). Using the 'magic' value of 0 on automaticallyAfter was designed to get around a limitation of being forced into the default rather than leaving it unset.

As of now, I do not think this change is necessary. There may still be a case for improving the doc string to be more descriptive of what the actual behavior is.

@scanlonp scanlonp added pr/do-not-merge This PR should not be merged at this time. wontfix We have determined that we will not resolve the issue. labels Oct 16, 2023
@go-to-k
Copy link
Contributor Author

go-to-k commented Oct 17, 2023

@scanlonp

OK, I will submit another PR for modifying the doc.

@go-to-k go-to-k closed this Oct 17, 2023
mergify bot pushed a commit that referenced this pull request Oct 18, 2023
…e is 0 is wrong (#27570)

We discussed that the doc when `automaticallyAfter` for `RotationSchedule` is `Duration.days(0)` is wrong. So I modified the doc.

See the issue (#27460) and another PR (#27497 (comment)).

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK bug This issue is a bug. effort/medium Medium work item – several days of effort p2 pr/do-not-merge This PR should not be merged at this time. pr/needs-maintainer-review This PR needs a review from a Core Team Member wontfix We have determined that we will not resolve the issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws_secretsmanager): automaticallyAfter is not disabling the automatic rotation of secrets
4 participants