-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
feat(ec2): add nitro enclave and hibernation settings to the Instance Construct #30228
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I made some comments!
* | ||
* @default false | ||
*/ | ||
readonly hibernationConfigured?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change to hibernationEnabled
to match enclaveEnabled
? (HibernationOptionsProperty
in L1 has the configured
, but ...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the property name.
[true, true], | ||
[false, false], | ||
])('given nitroEnclaveEnabled %p', (given: boolean, expected: boolean) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it needs to change the sentence If we change the parameter name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to match the parameter name.
[true, true], | ||
[false, false], | ||
])('given hibernationConfigured %p', (given: boolean, expected: boolean) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to match the parameter name.
}); | ||
|
||
test('throw if AWS Nitro Enclaves and hibernation are enabled', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to use parameter names in the sentence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed the sentence to match parameter names.
Co-authored-by: k.goto (Kenta Goto) <24818752+go-to-k@users.noreply.github.com>
Co-authored-by: k.goto (Kenta Goto) <24818752+go-to-k@users.noreply.github.com>
Co-authored-by: k.goto (Kenta Goto) <24818752+go-to-k@users.noreply.github.com>
Co-authored-by: k.goto (Kenta Goto) <24818752+go-to-k@users.noreply.github.com>
Co-authored-by: k.goto (Kenta Goto) <24818752+go-to-k@users.noreply.github.com>
Co-authored-by: k.goto (Kenta Goto) <24818752+go-to-k@users.noreply.github.com>
Co-authored-by: k.goto (Kenta Goto) <24818752+go-to-k@users.noreply.github.com>
Co-authored-by: k.goto (Kenta Goto) <24818752+go-to-k@users.noreply.github.com>
Co-authored-by: k.goto (Kenta Goto) <24818752+go-to-k@users.noreply.github.com>
@go-to-k |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the last comment (That is very very minor)!
Co-authored-by: k.goto (Kenta Goto) <24818752+go-to-k@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: rename file to integ.instance-nitro-enclaves-hibernation.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry typo. I fixed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mazyu36 , i see some of the files updated but not all in your last commit, you'll first need to rename the integ test file name to integ.instance-nitoro-enclaves-hibernation.ts, build and then run it to update file and its snapshot.
File name is still integ.instance-nitoro-envlaves-hibernation.ts
...-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-nitoro-envlaves-hibernation.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shikha372
Thanks.
I think the link you provided seems to be an old commit.
I renamed a integ test and reran it at this commit.
And in last commit, all files appear to be renamed.
I would appreciate it if you could point out any misunderstandings on my part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank mazyu, followed your last commit link, main integ test file name is still incorrect here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops... I misspelled "enclaves" too, sorry about that.
Thanks @mazyu36 for your contribution, looking at the documentation for this feature, shall we add some instance type based checks to ensure that this property is enabled only for specific type of instance ? |
@shikha372 However, I thought it would be inconsiderate to have no information at all, so I added an explanation and a link to the docs for enclaveEnabled. What do you think? Since I couldn't come up with many other good ideas, I would appreciate your feedback if you have any better suggestions. |
I see, thank you @mazyu36 for your contribution. Clear documentation will help in this case. |
@shikha372 If you have any better suggestions, I would appreciate your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: filename
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@shikha372 |
Issue # (if applicable)
N/A
Reason for this change
MIssing property in the L2 Construct
Description of changes
Add nitroEnclaveEnabled and hibernationConfigured property.
Description of how you validated changes
Added unit tests and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license