fix(secretsmanager): automatic rotation cannot be disabled

[AdamVD]

fixes #18749

Adds a manualRotation prop to RotationSchedule in order to leave automaticallyAfterDays unset on the underlying Cfn resource.

---

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

[gitpod-io]

[AdamVD]

@skinny85 just bumping this!

[skinny85]

@AdamVD thanks for the ping, this slipped under my radar!

Before I dive into the details, what do you think about, instead of adding a new property, adding a "magic" value for automaticallyAfter (0 comes to mind immediately), and that will mean "disable rotation"?

[AdamVD]

@skinny85 No problem, I figured that was all and that's why I bumped!

I actually proposed that as one of the two options in the issue #18749. Basically my argument for the solution you see implemented is that it is better from a UX perspective rather than using a "magic" value of zero. The issue creator agreed, but I'll follow your wisdom on what the better solution is out of the two. Just let me know!

[skinny85]

I actually feel like automaticallyAfter: Duration.days(0) makes it clear to me that rotation is disabled, and doesn't require a separate property (and all of the dance that you have to do to validate that they're not both provided at the same time).

I guess I don't know for a fact that Duration.days(0) is allowed 😛. Can you check?

[TheRealAmazonKendra]

I'm inclined to agree that using automaticallyAfter: Duration.days(0) is the preferred route. I'm not a big fan of using mutually exclusive optional parameters.

[skinny85]

@AdamVD are you still working on this PR?

[AdamVD]

@skinny85 Thanks for the bump. I'll try to switch the implementation by early next week.

Pull request has been modified.

[Jacco]

From a UX perspective maybe a Duration.never() could be implemented so that the user does not have to google if it’s 0 or 9999 :-)

[AdamVD]

@Jacco Interesting idea! I also have the feeling that a duration of zero isn't a great UX, but we are limited on options. I think a never option could make sense with something like Schedule (from aws-events), but it doesn't seem to belong with Duration. I wonder if adding Duration.zero() would be better than picking one of the time units to provide with 0. Using a union type would give us some good options but they can't be used in the CDK codebase.

@skinny85 I'll leave it to you to decide!

[skinny85]

I really like the ideas here!

Between Duration.never() and Duration.zero() - that's a tough call! I really like the "fluentness" of never(), but I agree it might be a tiny bit confusing.

@AdamVD I'll let you make the final call - propose something in the PR!

Also, to be 100% transparent, I'm no longer with AWS, so I can't merge this PR anymore - but I'd be happy to help in whatever way I can!

[Jacco]

Duration.eternity ;-)

[AdamVD]

@skinny85 thanks for the input and clarifying your status. Best wishes on your transition!

@Jacco I actually love that... "Automatically rotate my keys after eternity". It's perfect! I wonder if I can get someone to approve this 🤔

[mergify]

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
• Commit ID: 26793ea
• 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).