feat: allow customizing spot allocation strategy #481
Conversation
|
|
There was a problem hiding this comment.
Hi @vucong2409,
Thanks for taking a jab at this!
I left a couple comments/suggestions regarding the formatting of the docs and validation of the option, please let me know if you need some help or more explanation, otherwise feel free to address this feedback and I'll do another round of review then.
Thanks again!
vucong2409
force-pushed
the
feat/allow-custom-spot-allocation-strat
branch
2 times, most recently
from
b12598c to
b35667a
Compare
vucong2409
force-pushed
the
feat/allow-custom-spot-allocation-strat
branch
from
b35667a to
807b998
Compare
There was a problem hiding this comment.
Hi @vucong2409,
Sorry I forgot to do another pass of review after you updated this, I've left another review on the code, this looks good however I'm not sure the option should be common if it is ebs-specific, please let me know if something's unclear in my comments, and I can try to clarify.
Otherwise I'll let you address those comments, and I'll be back later for another round of reviews.
Thanks!
| @@ -251,6 +251,7 @@ func (b *Builder) Run(ctx context.Context, ui packersdk.Ui, hook packersdk.Hook) | |||
| SpotTags: b.config.SpotTags, | |||
| Tags: b.config.RunTags, | |||
| SpotInstanceTypes: b.config.SpotInstanceTypes, | |||
| SpotAllocationStrategy: b.config.RunConfig.SpotAllocationStrategy, | |||
There was a problem hiding this comment.
I believe this setting should be replicated to other builders, assuming this is compatible with them.
Otherwise I would suggest either:
- moving that argument to
builder/ebs/config.goso it is not common - checking that it is not set in the configs for the builders that aren't compatible with it, this can be done in the
Preparefunction for each builder.
There was a problem hiding this comment.
Note: if the ebs builder's the only one to support spot instances, I would suggest going with approach 1
| @@ -843,6 +851,13 @@ func (c *RunConfig) Prepare(ctx *interpolate.Context) []error { | |||
| } | |||
| } | |||
|
|
|||
| if c.SpotAllocationStrategy != "" { | |||
| if !slices.Contains(ec2.SpotAllocationStrategy_Values(), c.SpotAllocationStrategy) { | |||
There was a problem hiding this comment.
I believe both ifs can be mutualised
| if !slices.Contains(ec2.SpotAllocationStrategy_Values(), c.SpotAllocationStrategy) { | |
| if c.SpotAllocationStrategy != "" && !slices.Contains(ec2.SpotAllocationStrategy_Values(), c.SpotAllocationStrategy) { |
There was a problem hiding this comment.
Also to reply to your question in a comment from a previous review: using slices.Contains is fine by my standards! No need to make it manual, I suspect those were made this way in other cases because either the function did not exist at the time it was added, or whomever implemented it wasn't aware it existed, functionally it is essentially the same, might as well use what the library offers us.
Allow user to pass Spot Allocation Strategy into Fleet Request.
Closes #427