Skip to content

Commit

Permalink
Fix review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@panslava
panslava committed Sep 5, 2022
1 parent cb02f09 commit 494a2df
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 46 deletions.
3 changes: 1 addition & 2 deletions pkg/healthchecksl4/healthchecksl4.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ type l4HealthChecks struct {
// sharedResourceLock serializes operations on the healthcheck and firewall
// resources shared across multiple Services.
sharedResourcesLock sync.Mutex
hcProvider HealthChecksProvider
hcProvider healthChecksProvider
cloud *gce.Cloud
recorderFactory events.RecorderProducer
}
Expand Down Expand Up @@ -105,7 +105,6 @@ func GetInstance() *l4HealthChecks {
// Firewall rules are always created at in the Global scope (vs
// Regional). This means that one Firewall rule is created for
// Services of different scope (Global vs Regional).

func (l4hc *l4HealthChecks) EnsureHealthCheck(svc *corev1.Service, namer namer.L4ResourcesNamer, sharedHC bool, scope meta.KeyType, l4Type utils.L4LBType, nodeNames []string) *EnsureL4HealthCheckResult {
namespacedName := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace}

Expand Down
2 changes: 1 addition & 1 deletion pkg/healthchecksl4/interfaces.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ type EnsureL4HealthCheckResult struct {
Err error
}

type HealthChecksProvider interface {
type healthChecksProvider interface {
Get(name string, scope meta.KeyType) (*composite.HealthCheck, error)
Create(healthCheck *composite.HealthCheck) error
Update(name string, scope meta.KeyType, updatedHealthCheck *composite.HealthCheck) error
Expand Down
8 changes: 4 additions & 4 deletions pkg/healthchecksprovider/healthchecksprovider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@ func NewHealthChecks(cloud *gce.Cloud, version meta.Version) *HealthChecks {
}
}

func (hc *HealthChecks) createKey(name string, scope meta.KeyType) (*meta.Key, error) {
return composite.CreateKey(hc.cloud, name, scope)
}

func (hc *HealthChecks) Get(name string, scope meta.KeyType) (*composite.HealthCheck, error) {
key, err := hc.createKey(name, scope)
if err != nil {
Expand Down Expand Up @@ -84,3 +80,7 @@ func (hc *HealthChecks) SelfLink(name string, scope meta.KeyType) (string, error

return cloudprovider.SelfLink(meta.VersionGA, hc.cloud.ProjectID(), "healthChecks", key), nil
}

func (hc *HealthChecks) createKey(name string, scope meta.KeyType) (*meta.Key, error) {
return composite.CreateKey(hc.cloud, name, scope)
}
79 changes: 40 additions & 39 deletions pkg/healthchecksprovider/healthchecksprovider_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ func TestCreateHealthCheck(t *testing.T) {
desc string
}{
{
desc: "Test creating regional health check",
desc: "Create regional health check",
healthCheck: &composite.HealthCheck{
Name: "regional-hc",
Scope: meta.Regional,
},
},
{
desc: "Test creating global health check",
desc: "Create global health check",
healthCheck: &composite.HealthCheck{
Name: "global-hc",
Scope: meta.Global,
Expand All @@ -35,6 +35,7 @@ func TestCreateHealthCheck(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
t.Errorf("haha")
fakeGCE := gce.NewFakeGCECloud(gce.DefaultTestClusterValues())
hc := NewHealthChecks(fakeGCE, meta.VersionGA)

Expand Down Expand Up @@ -71,42 +72,42 @@ func TestGetHealthCheck(t *testing.T) {
desc string
}{
{
desc: "Test getting regional health check",
desc: "Get regional health check",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
getHCName: regionalHealthCheck.Name,
getHCScope: regionalHealthCheck.Scope,
expectedHealthCheck: regionalHealthCheck,
},
{
desc: "Test getting global health check",
desc: "Get global health check",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
getHCName: globalHealthCheck.Name,
getHCScope: globalHealthCheck.Scope,
expectedHealthCheck: globalHealthCheck,
},
{
desc: "Test getting non existent global health check",
desc: "Get non existent global health check",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
getHCName: "non-existent-hc",
getHCScope: meta.Global,
expectedHealthCheck: nil,
},
{
desc: "Test getting non existent regional health check",
desc: "Get non existent regional health check",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
getHCName: "non-existent-hc",
getHCScope: meta.Regional,
expectedHealthCheck: nil,
},
{
desc: "Test getting existent regional health check, but providing global scope",
desc: "Get existent regional health check, but providing global scope",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
getHCName: regionalHealthCheck.Name,
getHCScope: meta.Global,
expectedHealthCheck: nil,
},
{
desc: "Test getting existent global health check, but providing regional scope",
desc: "Get existent global health check, but providing regional scope",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
getHCName: globalHealthCheck.Name,
getHCScope: meta.Regional,
Expand Down Expand Up @@ -148,46 +149,46 @@ func TestDeleteHealthCheck(t *testing.T) {
}

testCases := []struct {
existingHealthChecks []*composite.HealthCheck
deleteHCName string
deleteHCScope meta.KeyType
shouldExistHealthChecks []*composite.HealthCheck
desc string
existingHealthChecks []*composite.HealthCheck
deleteHCName string
deleteHCScope meta.KeyType
shouldNotDeleteHealthChecks []*composite.HealthCheck
desc string
}{
{
desc: "Delete regional health check",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
deleteHCName: regionalHealthCheck.Name,
deleteHCScope: regionalHealthCheck.Scope,
shouldExistHealthChecks: []*composite.HealthCheck{globalHealthCheck},
desc: "Delete regional health check",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
deleteHCName: regionalHealthCheck.Name,
deleteHCScope: regionalHealthCheck.Scope,
shouldNotDeleteHealthChecks: []*composite.HealthCheck{globalHealthCheck},
},
{
desc: "Delete global health check",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
deleteHCName: globalHealthCheck.Name,
deleteHCScope: globalHealthCheck.Scope,
shouldExistHealthChecks: []*composite.HealthCheck{regionalHealthCheck},
desc: "Delete global health check",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
deleteHCName: globalHealthCheck.Name,
deleteHCScope: globalHealthCheck.Scope,
shouldNotDeleteHealthChecks: []*composite.HealthCheck{regionalHealthCheck},
},
{
desc: "Delete non existent healthCheck",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
deleteHCName: "non-existent",
deleteHCScope: meta.Regional,
shouldExistHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
desc: "Delete non existent healthCheck",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
deleteHCName: "non-existent",
deleteHCScope: meta.Regional,
shouldNotDeleteHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
},
{
desc: "Delete global health check name, but using regional scope",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
deleteHCName: globalHealthCheck.Name,
deleteHCScope: meta.Regional,
shouldExistHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
desc: "Delete global health check name, but using regional scope",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
deleteHCName: globalHealthCheck.Name,
deleteHCScope: meta.Regional,
shouldNotDeleteHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
},
{
desc: "Delete regional health check name, but using global scope",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
deleteHCName: regionalHealthCheck.Name,
deleteHCScope: meta.Global,
shouldExistHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
desc: "Delete regional health check name, but using global scope",
existingHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
deleteHCName: regionalHealthCheck.Name,
deleteHCScope: meta.Global,
shouldNotDeleteHealthChecks: []*composite.HealthCheck{regionalHealthCheck, globalHealthCheck},
},
}

Expand All @@ -206,7 +207,7 @@ func TestDeleteHealthCheck(t *testing.T) {
if err != nil {
t.Errorf("verifyHealthCheckNotExists(_, %s, %s) returned error %v", tc.deleteHCName, tc.deleteHCScope, err)
}
for _, hc := range tc.shouldExistHealthChecks {
for _, hc := range tc.shouldNotDeleteHealthChecks {
err = verifyHealthCheckExists(fakeGCE, hc.Name, hc.Scope)
if err != nil {
t.Errorf("verifyHealthCheckExists(_, %s, %s) returned error %v", hc.Name, hc.Scope, err)
Expand Down

0 comments on commit 494a2df

Please sign in to comment.