This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Enable "gosimple" linter plugin #3802

Merged

jackfrancis merged 4 commits into Azure:master from mboersma:linter-updates

Sep 7, 2018

Member

mboersma commented Sep 5, 2018 •

edited

Loading

What this PR does / why we need it:

Simplifies some code as suggested by the gosimple plugin to gometalinter and enables that plugin for the make test-style target.

~~(We can't run this plugin in general for linting, however, because it suggests some incorrect changes to pkg/api/types_test.go.)~~

Special notes for your reviewer:

Obviously not a critical change, but I've had this branch sitting around for a month so I freshened it up and made a PR.

If applicable:

documentation
unit tests
tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

NONE


          Simplify code as suggested by gosimple

9bdea9f

mboersma added the kind/chore label

mboersma self-assigned this

ghost added the in progress label

acs-bot added the approved label

jackfrancis removed the approved label

acs-bot added the size/M label

jackfrancis reviewed

View reviewed changes

pkg/acsengine/defaults_test.go

@@ @@ -618,12 +618,12 @@ func TestSetVMSSDefaults(t *testing.T) { @@
               	properties.AgentPoolProfiles[0].Count = 110
               	setPropertiesDefaults(&mockCS, false, false)
-              	if *properties.AgentPoolProfiles[0].SinglePlacementGroup != false {

Member

jackfrancis Sep 5, 2018

lol

jackfrancis reviewed

View reviewed changes

pkg/acsengine/defaults_test.go Outdated

@@ @@ -618,12 +618,12 @@ func TestSetVMSSDefaults(t *testing.T) { @@
               	properties.AgentPoolProfiles[0].Count = 110
               	setPropertiesDefaults(&mockCS, false, false)
-              	if *properties.AgentPoolProfiles[0].SinglePlacementGroup != false {
+              	if *properties.AgentPoolProfiles[0].SinglePlacementGroup {

Member

jackfrancis Sep 5, 2018

Do we need the * syntax here at all?

jackfrancis reviewed

View reviewed changes

pkg/acsengine/transform/apimodel_merger.go

@@ @@ -23,7 +23,7 @@ type APIModelValue struct { @@
               // MapValues converts an arraw of rwa ApiModel values (like ["masterProfile.count=4","linuxProfile.adminUsername=admin"]) to a map
               func MapValues(m map[string]APIModelValue, setFlagValues []string) {
-              	if setFlagValues == nil || len(setFlagValues) == 0 {
+              	if len(setFlagValues) == 0 {

Member

jackfrancis Sep 5, 2018

no... really, are you sure you're unset?

Member Author

mboersma Sep 6, 2018

pkg/acsengine/transform/apimodel_merger.go:26:5:warning: should omit nil check; len() for nil slices is defined as zero (S1009) (gosimple).

jackfrancis reviewed

View reviewed changes

pkg/api/agentPoolOnlyApi/v20180331/validate.go Outdated

@@ @@ -40,7 +40,7 @@ func (l *LinuxProfile) Validate() error { @@
               // Validate implements APIObject
               func (a *AADProfile) Validate(rbacEnabled *bool) error {
-              	if rbacEnabled == nil || *rbacEnabled == false {
+              	if rbacEnabled == nil || !*rbacEnabled {

Member

jackfrancis Sep 5, 2018

see helpers.IsTrueBoolPointer(), I think that's a more expressive way to put the above. E.g.:

if !helper.IsTrueBoolPointer(rbacEnabled) {...

jackfrancis reviewed

View reviewed changes

pkg/api/converterfromagentpoolonlyapi_test.go

@@ @@ -244,7 +244,6 @@ func TestConvertToV20180331AddonProfile(t *testing.T) { @@
               func TestConvertKubernetesConfigToEnableRBACV20180331AgentPoolOnly(t *testing.T) {
               	var kc *KubernetesConfig
-              	kc = nil

Member

jackfrancis Sep 5, 2018

lol I'm surprised the compiler accepts this

jackfrancis reviewed

View reviewed changes

pkg/api/convertertoagentpoolonlyapi.go Outdated

               }
               func convertV20180331AgentPoolOnlyKubernetesConfig(enableRBAC *bool) *KubernetesConfig {
-              	if enableRBAC != nil && *enableRBAC == true {
+              	if enableRBAC != nil && *enableRBAC {

Member

jackfrancis Sep 5, 2018

see my above suggestion

jackfrancis reviewed

View reviewed changes

pkg/api/convertertoagentpoolonlyapi_test.go Outdated

               		t.Error("OrchestratorProfile.KubernetesConfig expected not to be nil")
               	}
-              	if op.KubernetesConfig.EnableRbac == nil || *op.KubernetesConfig.EnableRbac == true {
+              	if op.KubernetesConfig.EnableRbac == nil || *op.KubernetesConfig.EnableRbac {

Member

jackfrancis Sep 5, 2018

ditto

jackfrancis reviewed

View reviewed changes

pkg/api/convertertoagentpoolonlyapi_test.go Outdated

               		t.Error("OrchestratorProfile.KubernetesConfig.EnableRbac expected to be *false")
               	}
-              	if op.KubernetesConfig.EnableSecureKubelet == nil || *op.KubernetesConfig.EnableSecureKubelet == true {
+              	if op.KubernetesConfig.EnableSecureKubelet == nil || *op.KubernetesConfig.EnableSecureKubelet {

Member

jackfrancis Sep 5, 2018

ditto

jackfrancis reviewed

View reviewed changes

pkg/api/convertertoagentpoolonlyapi_test.go

               		t.Error("OrchestratorProfile.KubernetesConfig.EnableSecureKubelet expected to be *false")
               	}
               }
               func TestConvertV20180331AgentPoolOnlyKubernetesConfig(t *testing.T) {
-              	var kc *KubernetesConfig
-              	kc = convertV20180331AgentPoolOnlyKubernetesConfig(helpers.PointerToBool(true))
+              	var kc = convertV20180331AgentPoolOnlyKubernetesConfig(helpers.PointerToBool(true))

Member

jackfrancis Sep 5, 2018

💥

jackfrancis reviewed

View reviewed changes

pkg/api/convertertoagentpoolonlyapi_test.go

               		t.Error("EnableRbac expected not to be nil")
               	}
-              	if *kc.EnableRbac != false {

Member

jackfrancis Sep 5, 2018

wow

jackfrancis reviewed

View reviewed changes

pkg/api/convertertoagentpoolonlyapi_test.go

               		t.Error("EnableRbac expected to be false")
               	}
               	if kc.EnableSecureKubelet == nil {
               		t.Error("EnableSecureKubelet expected not to be nil")
               	}
-              	if *kc.EnableSecureKubelet != false {

Member

jackfrancis Sep 5, 2018

ditto

jackfrancis reviewed

View reviewed changes

pkg/api/convertertoagentpoolonlyapi_test.go

               		t.Error("EnableRbac expected not to be nil")
               	}
-              	if *kc.EnableRbac != false {

Member

jackfrancis Sep 5, 2018

...

jackfrancis reviewed

View reviewed changes

pkg/api/convertertoagentpoolonlyapi_test.go

               		t.Error("EnableRbac expected to be false")
               	}
               	if kc.EnableSecureKubelet == nil {
               		t.Error("EnableSecureKubelet expected not to be nil")
               	}
-              	if *kc.EnableSecureKubelet != false {

Member

jackfrancis Sep 5, 2018

make it stop

Member Author

mboersma Sep 5, 2018

It stops here.

jackfrancis reviewed

View reviewed changes

pkg/armhelpers/graph.go

    
            @@ -112,7 +112,7 @@ func (az *AzureClient) CreateRoleAssignmentSimple(ctx context.Context, resourceG
          
              		},

              	}

              	re := regexp.MustCompile("(?i)status=(\\d+)")

              	re := regexp.MustCompile(`(?i)status=(\d+)`)

Member

jackfrancis Sep 5, 2018

hmm, that extra \, wouldn't that mean that that regex wouldn't be looking for a digit? How did the prior regex even work?

Member Author

mboersma Sep 5, 2018

The original regex is a normal, interpreted string literal, so it needed to escape the backslash. Using the backtics syntax makes it a raw string literal, so no escaping is needed. Raw string literals are preferable for regexes for readability and copy-and-pasting.

jackfrancis reviewed

View reviewed changes

test/e2e/engine/template.go

-              	}
-              	return false
+              	return strings.Contains(e.ExpandedDefinition.Properties.OrchestratorProfile.KubernetesConfig.NetworkPolicy, name)

Member

jackfrancis Sep 5, 2018

💥 💥

jackfrancis reviewed

View reviewed changes

test/e2e/kubernetes/deployment/deployment.go

               		select {
               		case err := <-errCh:
               			return pods, err
-              		case _ = <-readyCh:

Member

jackfrancis Sep 5, 2018

wut

Member Author

mboersma Sep 5, 2018

I guess you can elide the no-op assignment inside a select statement.

jackfrancis reviewed

View reviewed changes

test/e2e/kubernetes/pod/pod.go

    
            @@ -554,7 +554,7 @@ func (p *Pod) ValidateOmsAgentLogs(execCmdString string, sleep, duration time.Du
          
              // CheckWindowsOutboundConnection will keep retrying the check if an error is received until the timeout occurs or it passes. This helps us when DNS may not be available for some time after a pod starts.

              func (p *Pod) CheckWindowsOutboundConnection(sleep, duration time.Duration) (bool, error) {

              	exp, err := regexp.Compile("(StatusCode\\s*:\\s*200)")

              	exp, err := regexp.Compile(`(StatusCode\s*:\s*200)`)

Member

jackfrancis Sep 5, 2018

I think I'm getting it now that the surrounding quotes were escaping the slashes and then passing those into the regex compiler literally

mboersma added 3 commits

September 5, 2018 20:51


          Prefer helpers.IsTrueBoolPointer for checking variable truth

4e9ac75


          Promote some bool consts to vars in types_test.go

ca83a88

This silences the gosimple linter and preserves the readable
semantic intent of the test.


          Add "gosimple" plugin and increase linter timeout

98b8d7c

codecov bot commented Sep 6, 2018

Codecov Report

Merging #3802 into master will increase coverage by 0.05%.
The diff coverage is 20%.

@@            Coverage Diff            @@
##           master   #3802      +/-   ##
=========================================
+ Coverage   55.65%   55.7%   +0.05%     
=========================================
  Files         109     109              
  Lines       16233   16232       -1     
=========================================
+ Hits         9034    9042       +8     
+ Misses       6410    6398      -12     
- Partials      789     792       +3

acs-bot added approved and removed size/M labels

acs-bot added the size/L label

mboersma changed the title ~~Simplify code as suggested by gosimple~~ Enable "gosimple" linter plugin

mboersma added awaiting review and removed in progress labels

Member

jackfrancis commented Sep 7, 2018

/lgtm

acs-bot assigned jackfrancis

acs-bot added the lgtm label

acs-bot commented Sep 7, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, mboersma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jackfrancis,mboersma]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jackfrancis merged commit 8eda202 into Azure:master

ghost removed the awaiting review label

mboersma deleted the linter-updates branch

September 7, 2018 22:17

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved kind/chore lgtm size/L