-
Notifications
You must be signed in to change notification settings - Fork 69
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(serverGroup): Fixing asg rules and handler #293
Conversation
jeyrschabu
commented
Nov 6, 2019
- remove misleading classifiers for disabled. use fields instead
- fix test
- remove misleading classifiers for disabled. use fields instead - fix test
@@ -79,14 +79,6 @@ data class AmazonAutoScalingGroup( | |||
return getAddToLoadBalancerProcess()?.suspensionReason | |||
} | |||
|
|||
fun isDisabled(): 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.
@aravindmd this creates a circular dependency.
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 explain this bit more ?
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.
IS_DISABLED
is not a field on the amazon autoscaling group. It's something that the handler was responsible to set.
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 , that makes sense now.
return (details[IS_DISABLED] != null && details[IS_DISABLED] == true) | ||
} | ||
|
||
fun hasInstances(): 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.
See above comment
@@ -152,20 +151,7 @@ class AmazonAutoScalingGroupHandler( | |||
if (serverGroups == null || serverGroups.isEmpty()) { | |||
return | |||
} | |||
val elapsedTimeMillis = measureTimeMillis { |
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.
No need to do this here since we have all references already present. Load balancer and instances
@@ -72,7 +72,7 @@ class ZeroInstanceInDiscoveryDisabledServerGroupRule( | |||
private val disabledDurationInDays: Long = 30 | |||
|
|||
override fun apply(resource: AmazonAutoScalingGroup): Result { | |||
if (!resource.isDisabled()) { | |||
if (resource.isInLoadBalancer()) { |
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.
more explicit
createdTime = clock.millis() | ||
|
||
).apply { | ||
set(IS_DISABLED, false) |
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.
@aravindmd better to check actual fields on the server groups.
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.
LGTM