-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 Compute Operations to work identically & introduce a scope-less API #191
Refactor Compute Operations to work identically & introduce a scope-less API #191
Conversation
google/compute_operation.go
Outdated
case Region: | ||
op, err = w.Service.RegionOperations.Get(w.Project, scope, w.Op.Name).Do() | ||
case Zone: | ||
op, err = w.Service.ZoneOperations.Get(w.Project, scope, w.Op.Name).Do() | ||
default: |
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 doesn't seem like this condition could possibly be hit. What if the op
assignments were all done in the previous if/else statements?
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.
Done.
google/compute_operation.go
Outdated
return waitComputeOperationWaiter(w, timeoutMin, activity) | ||
} | ||
|
||
func waitComputeOperationWaiter(w *ComputeOperationWaiter, timeoutMin int, activity string) 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.
I don't have a great suggestion for you but I don't like that "wait" occurs twice in this fn name. waitCompute
? waitComputeOperation
could work but I don't like that there's a computeOperationWait
already. Or we can just leave it be since I don't think it'll need to be a separate fn once all the others are gone.
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.
Added a todo to make sure that we inline the method once the others are eliminated.
Thanks! Merge at will. |
…ess API (hashicorp#191) * Refactor compute_operation.go to duplicate less code. * Determine what scope type an Operation is from it's Operation object. * Inlined operation type switch statement into if/else methods.
…ess API (hashicorp#191) * Refactor compute_operation.go to duplicate less code. * Determine what scope type an Operation is from it's Operation object. * Inlined operation type switch statement into if/else methods.
<!-- This change is generated by MagicModules. --> /cc @rileykarson
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
We can use the
Region
andZone
properties of a computeOperation
to infer it's scope; we don't need to call scope-specific methods.compute_operation.go
to use the same code paths for all 3 operation typesWe can migrate existing resources over time with a series of followup CLs, eliminating the functions for each scope type as we go.
Smoke testing this with a resource of each scope:
Global:
Regional:
Zonal: