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

feat(deadline): Allow passing Context to SEP config #1211

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

mgway
Copy link
Contributor

@mgway mgway commented Jul 8, 2024

This change allows the reserved spot fleet context field to be passed to the spot event plugin configuration custom resource. One caveat that was introduced is the launch template must specify an explicit version instead of $Latest because of the launch template parameter constraint Context cannot be used with Launch Template version '$Latest' or '$Default'.

Added a test for this new parameter and updated the test for the launch template version number. The documentation for these fields is intentionally vague as the CFN construct for spot fleets does not document this field https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-spotfleet-spotfleetrequestconfigdata.html#cfn-ec2-spotfleet-spotfleetrequestconfigdata-context


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

@marofke
Copy link
Contributor

marofke commented Aug 22, 2024

Hi there, thanks for your submission! Sorry for the delayed response.

These changes look good to me, I'd just like a little more detail on your testing. Were you able to deploy your changes and confirm they worked as expected?

@marofke marofke self-requested a review August 22, 2024 17:24
@mgway
Copy link
Contributor Author

mgway commented Aug 22, 2024

I haven't yet tested these changes with rfdk 1.4.0 but a fork of 1.3.0 with these changes has been running a production farm for the past month without any ill effect and with the context string set in the spot requests as expected. The one thing I wasn't able to test was the behavior when an invalid context string is provided. However, I assume that it would work because configuring the spot fleet plugin via the UI will accept invalid context strings and only throw an error when the spot fleet request is eventually requested by the plugin.

Copy link
Contributor

@marofke marofke left a comment

Choose a reason for hiding this comment

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

Yeah that sounds reasonable to me, thank you for expanding on your testing, and for taking the time to put this change together!

We have a 2-person review process, so I'm having one of my colleagues to take a second look at this as well.

@marofke marofke merged commit 08b3672 into aws:mainline Aug 22, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants