-
Notifications
You must be signed in to change notification settings - Fork 1k
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
refactor(gce): Refactor BasicGoogleDeployHandler to be more readable and testable #6290
base: master
Are you sure you want to change the base?
Conversation
…change it to java
I published and deployed a snapshot version with my refactor and I validated a GCE deploy worked as expected. I didn't tests all possible configurations as part of my e2e validation. |
@Mergifyio refresh |
✅ Pull request refreshed |
@Mergifyio rebase |
☑️ Nothing to do
|
@@ -920,7 +920,7 @@ class GCEUtil { | |||
: [] | |||
} | |||
|
|||
static ServiceAccount buildScheduling(BaseGoogleInstanceDescription description) { | |||
static Scheduling buildScheduling(BaseGoogleInstanceDescription description) { |
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.
How did this ever work before?
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 line: https://github.com/spinnaker/clouddriver/blob/master/clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/deploy/GCEUtil.groovy#L924
It creates a Scheduling object in runtime. Groovy is flexible enough to allow a different object in runtime and it only pass the object to the InstanceProperties object:
https://github.com/spinnaker/clouddriver/blob/master/clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/deploy/handlers/BasicGoogleDeployHandler.groovy#L390
https://github.com/spinnaker/clouddriver/blob/master/clouddriver-google/src/main/groovy/com/netflix/spinnaker/clouddriver/google/deploy/handlers/BasicGoogleDeployHandler.groovy#L428
that is how this works but I don't like it. It's really hard to follow what is going on with groovy. That is why I moved it to java.
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.
OK. I suppose in some future we'll change GCEUtil to java too.
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.
yes, that is the plan
log.info( | ||
String.format( | ||
"Configuring explicit zones selected for regional server group: %s", | ||
description.getDistributionPolicy().getZones().get(0))); |
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.
get 0 - just the one? NOT checked old code... but we're streaming below which may mean multiple zones...
Key point about the refactor.
Related to: spinnaker/spinnaker#6985