Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Spotfleet Launch Template and On-Demand Capacity Support #4866
Spotfleet Launch Template and On-Demand Capacity Support #4866
Changes from 23 commits
69e71af
05d2327
52d7ebd
cd800bf
8587d5d
cd922e3
67bca1a
7cdb465
9384ef6
a3b4644
9310f97
0bedd0e
3092b71
7d93f45
3672583
ba51124
682500d
9b24059
19cd82f
c3e1c10
5c64ed3
7ccfc98
553b72f
0ba5456
3df1662
cb99970
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
In order to ensure this will never break, can we add an acceptance test to expect the conflicting error?
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.
@lossanarch Not sure to see the test on this one, were you able to check 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.
Hey I believe this one is covered by
TestAccAWSSpotFleetRequest_launchTemplateConflictLaunchSpecification
, line 166 of the tests file.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.
This ConflictsWith value will not work since
launch_template_configs
is of TypeSet, and this syntax is allowed by TypeList only.I'm thinking this should be handled in the CustomizeDiff instead, as it would allow to do it in a more efficient way.
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 only have 255 versions?
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 think this may have meant to be for a "version_description" attr, which is limited to 255 characters. The api lists "version" as type long though, so this definitely needs fixing.
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 seems this is missing the
priority
attribute as per https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_LaunchTemplateOverrides.html. Can you add that in, along with a test?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.
Do we need to mark this as computed? what about the computed attribute for
subnet_id
andweighted_capacity
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.
Not sure we need this one: the default hash function handles pretty much every case.
Is there a need for this one on your side?
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 think this is required because we have to add the launch template overrides to it, if they exist:
but it's been a super long time since I wrote this and I don't fully remember.
Is there a way to do this with the default hash function?
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 think with my comment at https://github.com/terraform-providers/terraform-provider-aws/pull/4866/files#diff-cd8c91a4c915b1ae56ba89540a3412dcR1135, we may not need this Hash Function at all. Can you give a try of fixing the code below?
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.
Can you add a comment explaining the need for that? just for future readers to better understand
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 actually stole this from the launch template implementation in the autoscaling group - I've commented it as how I understand it works, hopefully I'm right. 😬
Edit: Commit reference: 167c38f
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.
This wouldn't work since
launch_template_configs
is of TypeSet and the syntax used here is for TypeList withMaxItems: 1
.In this case, I would advise to iterate over the elements and check for the value using Go instead of HCL-based access. Let me know if that doesn't make sense!
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.
Also, since
launch_template_specification
is Computed and Optional, this seems extraneous, since the differences would never show if not configuredThere 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.
Could we rewrite this using the same logic as lines 800-806?
We also need to clean the comments if unused.
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.
Cleaned and logic standardised. Commit 127425f
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 feel we should be more specific about the message erroring, so that we can better handle different kind of errors.
We could rewrite part of this switch for something like:
but instead of
LaunchTemplateSpecification
, having a part of the sentence that matches a given error. Does that make sense?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 add a comment about why we can't retry on
LaunchTemplateSpecification
?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.
Comment added, commit 95e2fc6
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.
After thinking about it, we definitely need to check the output of this Set function here, and I would advise to switch to a more common approach that is more standard from a TF schema perspective:
What that means is
flattenLaunchTemplateConfigs
would be a function relying onflattenFleetLaunchTemplateSpecification
and theconfig.LaunchTemplateConfigs[0].Overrides
existence and value.flattenLaunchTemplateConfigs
would also handle the nil case line 1138, thus there wouldn't be a need for this check at all.In the end, it could be something like:
instead of:
There are several examples of that in
structure.go
, with an example