feat(ec2): adds persist option to user data on windows instances #21709

ddneilson · 2022-08-22T14:26:09Z

To set the UserData for Windows instances to run on every instance start, rather than only the first startup, requires adding "true" to the end of the user data string. Currently, users must use an escape hatch to hack the userdata of
windows instances to add this string. This PR makes it possible to add this text directly on the UserData without using an escape hatch.

Implements: #21708

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

gitpod-io · 2022-08-22T14:26:22Z

ddneilson · 2022-08-22T14:38:06Z

Regarding the PR Linter failure -- not adding an integ test. Does it really make sense to add one for this PR? It's not changing/adding any resources/constructs; it's adding functionality to a helper class.

TheRealAmazonKendra

Thank you for your contribution! I have a couple comments inline. In regard to the integration test, I'd like to see one where this function is called for the construct to show both that the output is what we expect when it's called and also to ensure that this deploys successfully when the function is used (i.e. running the test without using --dry-run).

TheRealAmazonKendra · 2022-08-22T16:29:40Z

packages/@aws-cdk/aws-ec2/lib/user-data.ts

+   *
+   * @param value If true then the userdata is persisted. (Windows only)
+   */
+  public abstract persist(value: boolean): void;


Given that half of the locations of this function throw errors when called, I don't think this is the right level of abstraction. I'm also not sure about this contract as a whole. Since this is a function, not a field, calling it implies that the user wants their data to persist. Given that, it seem that the boolean should be unnecessary.

I completely agree, however the CDK does not export the internal LinuxUserData or WindowsUserData classes. I originally just wanted to add the property to WindowsUserData, but then of course a reference to UserData trying to touch that property fails type checks. Without WindowsUserData being exported it's not even possible to do instance.userData as WindowsUserData

So, we have two options.

Accept that the property is on the base UserData class and only makes sense for one of the options; or

Export at least WindowsUserData so that users can type-cast to be able to set persist on it.

I went with (1), obviously. What would the CDK team prefer?

@TheRealAmazonKendra Before the bot kills this PR. Any response to my question?

@TheRealAmazonKendra Still waiting on your thoughts? Should I just add the integ test to the current PR's form, or switch to exporting WindowsUserData and only having the new persist method on WindowsUserData instead of all UserData classes?

I think the best option is actually to add this as an option in forWindows. forLinux has an option added it that is only specific to it so I think duplicating that experience makes sense in this context.

The suggestion has been implemented.

packages/@aws-cdk/aws-ec2/lib/user-data.ts

aws-cdk-automation · 2022-09-13T00:12:18Z

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.

Pull request has been modified.

TheRealAmazonKendra · 2022-09-17T03:37:21Z

Manually merged to reset the bot. I'm being a hypocrite here because I ask people to not do this, but I had missed your questions so the staleness is my fault, not yours.

TheRealAmazonKendra · 2022-10-02T05:07:47Z

@Mergifyio update

mergify · 2022-10-02T05:08:20Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra

Putting this back in changes requested now that I've provided an answer.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

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

PRs must pass status checks before we can provide a meaningful review.

To set the UserData for Windows instances to run on every instance start, rather than only the first startup, requires adding "<persist>true</persist>" to the end of the user data string. Currently, users must use an escape hatch to hack the userdata of windows instances to add this string. This PR makes it possible to add this text directly on the UserData without using an escape hatch.

Pull request has been modified.

packages/@aws-cdk/aws-ec2/lib/user-data.ts

TheRealAmazonKendra

Looks good! I removed some some extra spacing in a couple places but this looks good to go.

mergify · 2022-10-12T21:37:01Z

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

aws-cdk-automation · 2022-10-12T22:15:14Z

AWS CodeBuild CI Report

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

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

mergify · 2022-10-12T22:15:34Z

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

github-actions bot added the p2 label Aug 22, 2022

aws-cdk-automation requested a review from a team August 22, 2022 14:26

ddneilson changed the title ~~fix(ec2): adds persist option to user data~~ feat(ec2): adds persist option to user data Aug 22, 2022

TheRealAmazonKendra previously requested changes Aug 22, 2022

View reviewed changes

ddneilson requested a review from TheRealAmazonKendra August 23, 2022 14:38

TheRealAmazonKendra previously requested changes Oct 2, 2022

View reviewed changes

aws-cdk-automation previously requested changes Oct 2, 2022

View reviewed changes

ddneilson force-pushed the ddneilson/userdata_persist branch from 2fe66d7 to 2a55cdb Compare October 3, 2022 01:52

Naumel added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Oct 3, 2022

ddneilson requested a review from TheRealAmazonKendra October 3, 2022 14:12

TheRealAmazonKendra reviewed Oct 12, 2022

View reviewed changes

packages/@aws-cdk/aws-ec2/lib/user-data.ts Outdated Show resolved Hide resolved

Update packages/@aws-cdk/aws-ec2/lib/user-data.ts

7d054e9

TheRealAmazonKendra reviewed Oct 12, 2022

View reviewed changes

packages/@aws-cdk/aws-ec2/lib/user-data.ts Outdated Show resolved Hide resolved

Update packages/@aws-cdk/aws-ec2/lib/user-data.ts

8f39017

TheRealAmazonKendra changed the title ~~feat(ec2): adds persist option to user data~~ feat(ec2): adds persist option to user data on windows instances Oct 12, 2022

TheRealAmazonKendra approved these changes Oct 12, 2022

View reviewed changes

Merge branch 'main' into ddneilson/userdata_persist

a638eb3

mergify bot merged commit cb1506f into aws:main Oct 12, 2022

ddneilson deleted the ddneilson/userdata_persist branch October 13, 2022 00:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ec2): adds persist option to user data on windows instances #21709

feat(ec2): adds persist option to user data on windows instances #21709

ddneilson commented Aug 22, 2022

gitpod-io bot commented Aug 22, 2022

ddneilson commented Aug 22, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Aug 22, 2022

ddneilson Aug 22, 2022

ddneilson Sep 14, 2022

ddneilson Sep 27, 2022

TheRealAmazonKendra Oct 2, 2022

ddneilson Oct 3, 2022

aws-cdk-automation commented Sep 13, 2022

TheRealAmazonKendra commented Sep 17, 2022

TheRealAmazonKendra commented Oct 2, 2022

mergify bot commented Oct 2, 2022

TheRealAmazonKendra left a comment

aws-cdk-automation left a comment

TheRealAmazonKendra left a comment

mergify bot commented Oct 12, 2022

aws-cdk-automation commented Oct 12, 2022

mergify bot commented Oct 12, 2022

feat(ec2): adds persist option to user data on windows instances #21709

feat(ec2): adds persist option to user data on windows instances #21709

Conversation

ddneilson commented Aug 22, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Aug 22, 2022

ddneilson commented Aug 22, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 22, 2022

Choose a reason for hiding this comment

ddneilson Aug 22, 2022

Choose a reason for hiding this comment

ddneilson Sep 14, 2022

Choose a reason for hiding this comment

ddneilson Sep 27, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Oct 2, 2022

Choose a reason for hiding this comment

ddneilson Oct 3, 2022

Choose a reason for hiding this comment

aws-cdk-automation commented Sep 13, 2022

TheRealAmazonKendra commented Sep 17, 2022

TheRealAmazonKendra commented Oct 2, 2022

mergify bot commented Oct 2, 2022

✅ Branch has been successfully updated

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 12, 2022

aws-cdk-automation commented Oct 12, 2022

AWS CodeBuild CI Report

mergify bot commented Oct 12, 2022