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
GCE: Add functions for Alpha address and forwarding rules #51138
Conversation
just curious, do you need experimental flag for the alpha apis? |
I was planning to add that later, after your PR gets merged. |
/lgtm I would prefer not renaming compute to computev1 to reduce change |
@@ -105,6 +113,18 @@ func (gce *GCECloud) CreateRegionForwardingRule(rule *compute.ForwardingRule, re | |||
return gce.waitForRegionOp(op, region, mc) | |||
} | |||
|
|||
// CreateAlphaRegionForwardingRule creates and returns an Alpha | |||
// forwarding fule in the given region. | |||
func (gce *GCECloud) CreateAlphaRegionForwardingRule(rule *computealpha.ForwardingRule, region 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.
do you want to do something like this?
func (gce *GCECloud) createRegionForwardingRuleInternal(rule gceObject, region string) {
mc := newForwardingRuleMetricContext("create", region)
switch v := rule.(type) {
.............
}
return gce.waitForRegionOp(op, region, mc)
}
func (gce *GCECloud) CreateRegionForwardingRule(rule *computev1.ForwardingRule, region string) error {
return createRegionForwardingRuleInternal(rule, region)
}
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.
IMO, that doesn't seem to help.
/approve no-issue no strong opinion though since you only change one. |
@bowei What do you think about specifying "alpha_" or "beta_" in the metrics for each API func? Also, I agree about not renaming compute to computev1. |
Add "alpha_" or "beta_" to the metrics makes sense. |
/lgtm cancel |
I actually did that previously in the POC: yujuhong@7d58ea2 |
@freehan's previous PRs renamed the imports. I was trying to be consistent. will remove. |
I like having its own field even better. |
Reverted the package renaming in the first commit. Added a new commit for recording the version. PTAL |
look ok to me, nick has the final lgtm |
Looks awesome, thanks! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei, freehan, nicksardo, yujuhong Associated issue requirement bypassed by: freehan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Forgot about bazel. Updated the BUILD file and re-applied lgtm. |
/test pull-kubernetes-e2e-gce-etcd3 |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
No description provided.