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

fix: Ignore mixedInstancePolicy if type is LaunchConfig #303

Merged

Conversation

eytan-avisror
Copy link
Collaborator

Signed-off-by: Eytan Avisror eytan_avisror@intuit.com

Fixes #301

This ignores mixedInstancePolicy spec if it is used along with LaunchConfiguration as spec type.

Eytan Avisror added 2 commits May 11, 2021 12:45
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
@eytan-avisror eytan-avisror requested review from a team as code owners May 11, 2021 20:05
@eytan-avisror eytan-avisror changed the title Ignore mixedInstancePolicy if type is LaunchConfig fix: Ignore mixedInstancePolicy if type is LaunchConfig May 11, 2021
@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #303 (83d45b0) into master (a1ee5ca) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #303      +/-   ##
==========================================
- Coverage   70.11%   70.06%   -0.05%     
==========================================
  Files          19       19              
  Lines        3105     3107       +2     
==========================================
  Hits         2177     2177              
- Misses        788      789       +1     
- Partials      140      141       +1     
Impacted Files Coverage Δ
api/v1alpha1/instancegroup_types.go 15.13% <0.00%> (-0.10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a1ee5ca...83d45b0. Read the comment docs.

@@ -392,6 +392,13 @@ func (s *EKSSpec) Validate() error {
s.Type = LaunchConfiguration
}

if s.Type == LaunchConfiguration {
if s.EKSConfiguration.MixedInstancesPolicy != nil {
log.Info("cannot use mixedInstancesPolicy with LaunchConfiguration, will ignore provided mixedInstancePolicy")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this log is going to be very useful - does it contain any identifying information for the IG?

Might also make sense to event this on the IG, for users that aren't cluster operators.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we should pass the IG name from the top level Validate() so we can add the IG name.
Regarding events - yeah, that's a great idea to have events for any IG error associated with the IG for visibility - we might want to do that as a separate effort though across the board.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Created #304 to address events

Eytan Avisror added 4 commits May 11, 2021 15:04
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
…/instance-manager into mixed-instances-validation

Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
Signed-off-by: Eytan Avisror <eytan_avisror@intuit.com>
@eytan-avisror eytan-avisror merged commit 345ef0c into keikoproj:master May 12, 2021
@backjo backjo mentioned this pull request Jun 4, 2021
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.

Validation: block using mixedInstancePolicy with LaunchConfiguration as default
2 participants