Swtich to use beta HealthCheck for NEG #366
Conversation
k8s-ci-robot
added
cncf-cla: yes
freehan
force-pushed
the
neg-beta-healthcheck
branch
2 times, most recently
from
b0bf889
to
6e76504
Compare
pkg/healthchecks/healthchecks.go
Outdated
| glog.V(2).Infof("Updating health check for port %v with protocol %v", newHC.Port, newHC.Type) | ||
| v1hc, err := newHC.ToComputeHealthCheck() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return h.cloud.UpdateHealthCheck(v1hc) | ||
| default: | ||
| return fmt.Errorf("Unkonwn Version: %q", newHC.Version()) |
There was a problem hiding this comment.
nit: Unknown . I think it also is a typo earlier in this file
pkg/healthchecks/healthchecks.go
Outdated
| var hc *computealpha.HealthCheck | ||
| var err error | ||
| if alpha { | ||
| if version == meta.VersionAlpha { | ||
| hc, err = h.cloud.GetAlphaHealthCheck(name) |
There was a problem hiding this comment.
This part of the if clause is missing the:
if err != nil {
return nil, err
}
Alternatively, just do this check once at the end of this function, and otherwise return NewHealthCheck(hc), err
There was a problem hiding this comment.
irrelevant, just out of curiosity, why not switch .. case .. here?
There was a problem hiding this comment.
oh yeah. I had switch .. case .. at other places. Will make the change
pkg/healthchecks/healthchecks.go
Outdated
| hc.merge() | ||
| return toBetaHealthCheck(&hc.HealthCheck) | ||
| } | ||
|
|
||
| // ToComputeHealthCheck returns a valid compute.HealthCheck object |
There was a problem hiding this comment.
nit, this should also be ToAlphaComputeHealthCheck
pkg/healthchecks/healthchecks.go
Outdated
| err := copyViaJSON(ret, hc) | ||
| return ret, err | ||
| } | ||
|
|
||
| // toV1HealthCheck converts v1 health check to alpha health check. |
There was a problem hiding this comment.
nit, // betaToAlphaHealthCheck converts beta health check to alpha health check
pkg/healthchecks/healthchecks.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| hc, err = betaToAlphaHealthCheck(betaHC) | ||
| } |
There was a problem hiding this comment.
Should also check for invalid version here - return fmt.Errorf("Unknown Version: %q", hc.Version()). Since this is used in a few places, it could be a const too.
pkg/healthchecks/healthchecks.go
Outdated
| @@ -183,7 +203,7 @@ func mergeHealthcheck(oldHC, newHC *HealthCheck) *HealthCheck { | |||
| } | |||
|
|
|||
| func (h *HealthChecks) getHealthCheckLink(name string, alpha bool) (string, error) { | |||
There was a problem hiding this comment.
We need to change the method signature of this function to version meta.Version also. Below line should be h.Get(name, version).
freehan
force-pushed
the
neg-beta-healthcheck
branch
2 times, most recently
from
fc7ec2c
to
93c376f
Compare
freehan
force-pushed
the
neg-beta-healthcheck
branch
from
93c376f
to
94e3c1c
Compare
cc @agau4779