Staticcheck cleanup #4751

vytautas-karpavicius · 2022-02-24T10:03:50Z

What changed?
Cleaned up lots of code according to staticcheck suggestions. Each commits deals with one suggestion at a time. I have included suggestion code which can be looked up here: https://staticcheck.io/docs/checks

Those changes are mostly mechanical. There are more warning the tool reports, that may affect functionality. I will try to address those separately.

Why?
To clean up our codebase and make more consistent with best go practices.

How did you test it?

Potential risks

Release notes

Documentation Changes

…fix (S1017)

… (S1008)

…r.TreeID] (S1002)

…estors...) (S1011)

…gle case (S1000)

…t.TODO if you are unsure about which Context to use (SA1012)

… type to avoid collisions (SA1029)

…/cadence into staticcheck-cleanup

coveralls · 2022-02-24T14:18:24Z

Pull Request Test Coverage Report for Build 1f83e6c5-190c-4e80-aa95-78a09dcf5978

91 of 402 (22.64%) changed or added relevant lines in 70 files are covered.
30 unchanged lines in 10 files lost coverage.
Overall coverage increased (+0.1%) to 56.98%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
common/elasticsearch/client_v6.go	0	1	0.0%
common/elasticsearch/client_v7.go	0	1	0.0%
common/persistence/executionManager.go	5	6	83.33%
common/persistence/nosql/nosqlQueueStore.go	0	1	0.0%
common/persistence/nosql/nosqlplugin/cassandra/tasks.go	0	1	0.0%
common/persistence/sql/common.go	0	1	0.0%
common/persistence/sql/sqlDomainStore.go	0	1	0.0%
common/persistence/sql/sqlExecutionStore.go	1	2	50.0%
common/persistence/sql/sqlQueueStore.go	0	1	0.0%
common/persistence/sql/sqldriver/connections.go	0	1	0.0%

Files with Coverage Reduction	New Missed Lines	%
cmd/server/cadence/cadence.go	1	7.64%
common/dynamicconfig/configstore/config_store_client.go	1	79.43%
common/persistence/persistence-tests/persistenceTestBase.go	1	8.88%
common/task/weightedRoundRobinTaskScheduler.go	1	89.64%
common/task/fifoTaskScheduler.go	2	85.57%
service/history/queue/timer_queue_processor.go	2	58.37%
service/matching/taskListManager.go	2	74.19%
tools/cli/adminElasticSearchCommands.go	2	0%
common/persistence/nosql/nosqlExecutionStore.go	5	62.65%
common/persistence/nosql/nosqlplugin/cassandra/workflow.go	13	53.98%

Totals
Change from base Build 860d5573-8102-4be3-9fd2-0a5813436b29:	0.1%
Covered Lines:	83200
Relevant Lines:	146015

💛 - Coveralls

Groxx · 2022-02-24T22:43:09Z

common/archiver/gcloud/queryParser.go

-func (p *queryParser) convertCloseTime(timestamp int64, op string, parsedQuery *parsedQuery) error {
-	switch op {
-	case "=":
-		if err := p.convertCloseTime(timestamp, ">=", parsedQuery); err != nil {
-			return err
-		}
-		if err := p.convertCloseTime(timestamp, "<=", parsedQuery); err != nil {
-			return err
-		}
-	default:
-		return fmt.Errorf("operator %s is not supported for close time", op)
-	}
-	return nil
-}
-


unused or something? afaik this is correct for BigQuery-style datastores

Yeah this is unused.

Groxx · 2022-02-24T22:56:09Z

common/archiver/s3store/historyArchiver.go

@@ -68,7 +68,6 @@ type (
 		s3cli     s3iface.S3API
 		// only set in test code
 		historyIterator archiver.HistoryIterator
-		config          *config.S3Archiver


apparently this config is now contained in the s3cli, so 👍

Groxx · 2022-02-24T22:57:14Z

common/archiver/util.go

-	errEmptyDomainName       = errors.New("Domain name is empty")
+	errEmptyDomainName       = errors.New("DomainName is empty")


staticcheck complained about this? or did it just complain about uppercase -> now it matches the param name, and staticcheck is happy because it looks like a ProperNoun or something?

staticcheck complained about upper case. Since other errors here include exact param names, I changed this for consistency as well.

Out of curiosity checked their source code: https://github.com/dominikh/go-tools/blob/f41ebe766a412094c088d7d32598c19442db66c1/stylecheck/lint.go#L418

Seems like if it detects more upper letters in the first word - it is ok:

// Word is probably an initialism or // multi-word function name

Groxx · 2022-02-24T22:59:05Z

common/config/metrics.go

-	statter, err := statsd.NewBufferedClient(config.HostPort, config.Prefix, config.FlushInterval, config.FlushBytes)
+	statter, err := statsd.NewClientWithConfig(&statsd.ClientConfig{


ah. deprecated. 👍

Groxx · 2022-02-24T23:00:27Z

common/elasticsearch/esql/globals.go

-func print(v interface{}) {
-	fmt.Println("==============")
-	fmt.Println(v)
-	fmt.Println("==============")
-}


honestly I wonder if we can find a linter to just ban fmt.Print* across the board. it's basically always better-served by some injected thing, like a logger

Quickly searched for such usages. Majority of the comes in CLI or in tests. Those are probably valid uses.

Groxx · 2022-02-24T23:01:13Z

common/log/loggerimpl/logger_test.go

@@ -49,8 +49,7 @@ func TestDefaultLogger(t *testing.T) {
 		outC <- buf.String()
 	}()

-	var zapLogger *zap.Logger
-	zapLogger = zap.NewExample()
+	zapLogger := zap.NewExample()


would it cause any problems to change these to zaptest.New(t) instead? then logs will be scoped to the test.

Tried changing this - tests fail.

I think zaptest.New(t) is good for places when you just want to pass some logger when testing things and do no pollute output for passed tests. There are some places that could be updated to use it.

Here however, we testing the logger implementation itself.

Groxx · 2022-02-24T23:04:23Z

common/persistence/dataStoreInterfaces.go

-		RetentionSeconds   time.Duration
+		RetentionPeriod    time.Duration


minor honestly, but do we have any broad pattern for this kind of thing for "period" vs "interval"? since it's a bit odd to do it two ways in the same file.

(not that I think either is wrong, they kinda are different meanings. just wondering out loud if consistency would be better.)

Yeah I thought about it.

But if we decide to do it, we should change it everywhere. Period mostly goes with workflow retention, Interval is very common for retry policy fields. We also have lots of Timeout fields as well.

This would be big change. I'm not against it, but IMHO this is very low priority item.

Groxx · 2022-02-24T23:11:54Z

common/persistence/sql/sqlExecutionStoreUtil.go

-	if err := assertCurrentExecution(ctx, tx, shardID, domainID, workflowID, assertFn); err != nil {
-		return err
-	}


since this does a DB lock: is this something we should be calling, or is it being handled somewhere else?

Groxx · 2022-02-24T23:15:55Z

common/persistence/sql/sqlplugin/postgres/db.go

-		return true
-	}
-	return false
+	return err == sql.ErrNoRows


since we're working on wrapped errors: are you on a recent master? I'm not sure what the sequencing of these changes is. since this'd be a fairly obvious errors.Is(err, sql.ErrNoRows) transformation.
(but probably not worth doing in this PR, keep this one full of obviously-noops)

This is on recent master.
Yeah, lets leave it a separate PR, that addresses such cases.

Groxx · 2022-02-24T23:18:44Z

common/testing/event_generator.go

-func (g *EventGenerator) shouldBumpVersion() bool {
-	// 1//1000 to bump the version
-	//return g.dice.Intn(1000) == 500
-	return false
-}
-


huh. wonder if this would be useful to enable in tests. (but if unused: 👍)

Groxx · 2022-02-24T23:22:26Z

host/integration_test.go

-				binary.Write(buf, binary.LittleEndian, i)
+				binary.Write(buf, binary.LittleEndian, int32(i))


any reason for int32 vs int64? I think this was behaving as int64, as int is architecture-dependent and basically nobody uses 32-bit for servers

Good point, apparently I'm living in the past :)
Updated.

Groxx · 2022-02-24T23:25:09Z

host/reset_workflow_test.go

-	events = s.getHistory(s.domainName, &types.WorkflowExecution{
+	s.getHistory(s.domainName, &types.WorkflowExecution{


With errcheck (or similar), I guess this is probably preferable over _ = s.getHistory(...), isn't it... since that could be ignoring an error, but s.getHistory(...) cannot. Interesting.

Groxx · 2022-02-24T23:27:39Z

host/xdc/integration_failover_test.go

-//go:build !race
-// +build !race


that aside: could that be marking this as incorrectly-unused? and/or is there anything worth saving in here? we should just fix the races.

This entire test suite is unused and disabled for 10 months already.
AFAICT we have another similar suite in host/ndc. Might be worth double checking with the team.

service/frontend/clusterRedirectionHandler_test.go

Groxx · 2022-02-25T00:16:33Z

service/history/historyEngine2_test.go

@@ -848,6 +848,7 @@ func (s *engine2Suite) createExecutionStartedState(we types.WorkflowExecution, t
 }

 //nolint:unused
+//lint:ignore U1000 for printing within tests


huh. is it complaining because of the name? maybe better to just change the name...

This is currently not used anywhere, so we could remove it entirely.
Since I saw previous nolint I assumed this may be sometimes used for debugging in tests, so added additional declaration for staticcheck.
Not sure whether it is worth keeping such helpers though...

Groxx · 2022-02-25T00:24:48Z

service/matching/taskListManager_test.go

-	require.Error(t, errRemoteSyncMatchFailed) // should not persist the task
+	require.Error(t, err) // should not persist the task


lol. well that's a more useful test now

Groxx · 2022-02-25T00:29:21Z

service/worker/scanner/data_corruption_workflow_test.go

@@ -43,6 +43,8 @@ import (

 const testWorkflowName = "default-test-workflow-type-name"

+var validBranchToken = []byte{89, 11, 0, 10, 0, 0, 0, 12, 116, 101, 115, 116, 45, 116, 114, 101, 101, 45, 105, 100, 11, 0, 20, 0, 0, 0, 14, 116, 101, 115, 116, 45, 98, 114, 97, 110, 99, 104, 45, 105, 100, 0}


👍👍 for more valid values, though is there a more human-friendly way to build this? as it apparently includes "test-tree-id" and "test-branch-id" strings

Copied it from some other place, as the test actually started failing after checking for errors :)

This is thrift serialized structure underneath. I guess we could have helpers in our test to generate those.

Groxx · 2022-02-25T00:36:02Z

tools/cli/util.go

 	match, err := regexp.MatchString(defaultDateTimeRangeShortRE, timeRange)
 	if !match { // fallback on to check if it's of longer notation
-		match, err = regexp.MatchString(defaultDateTimeRangeLongRE, timeRange)
+		_, err = regexp.MatchString(defaultDateTimeRangeLongRE, timeRange)
 	}


not necessarily relevant as this doesn't change behavior, but: what if the second doesn't match? looks like regexp.MatchString will only error if the regex fails to compile.

more generally, it seems pretty obvious that we should only be compiling these once, rather than on every call :\ think that's worth doing here, or later?

Good observation. Few problems here:

regexes are not compiled as you mentioned (although not very impactful as those are used only on CLI commands)

these matches are not used anywhere. Error simply check whether they compile.

Match groups in those regexes are not utilized, we parse things manually below

I think I will address this in a separate PR. Also checked that there exists a library for such functionality: https://github.com/karrick/tparse
What is opinion on taking this dependency? Or just simply fixing is enough?

Groxx · 2022-02-25T00:42:07Z

notes are worth checking, but LGTM either way, and they're nearly all just observations / questions. Let me know if you'd like me to check any changes you do end up making :)

Co-authored-by: Steven L <imgroxx@gmail.com>

vytautas-karpavicius added 29 commits February 22, 2022 12:42

unnecessary use of fmt.Sprintf (S1039)

0d13208

could eliminate this type assertion (S1034)

5ed4aee

package is being imported more than once (ST1019)

b2d393a

redundant return statement (S1023)

de43cb0

should use make(...) instead (S1019)

aff55fc

should omit nil check; len() for nil slices is defined as zero (S1009)

bf3a603

should merge variable declaration with assignment on next line (S1021)

b12ec6e

should use fmt.Fprintf instead of fmt.Fprint(fmt.Sprintf(...)) (S1038)

71dc77f

hould replace this if statement with an unconditional strings.TrimPre…

ab75516

…fix (S1017)

should use bytes.Equal(data, data2) instead (S1004)

b80849d

should use 'return X' instead of 'if X { return true }; return false'…

e1f8e61

… (S1008)

should omit comparison to bool constant, can be simplified to trees[b…

a03daba

…r.TreeID] (S1002)

should replace loop with ancestors = append(ancestors, branchInfo.Anc…

f74777a

…estors...) (S1011)

should use a simple channel send/receive instead of select with a sin…

aecde22

…gle case (S1000)

value of type int cannot be used with binary.Write (SA1003)

9d59bd8

do not pass a nil Context, even if a function permits it; pass contex…

aaf593a

…t.TODO if you are unsure about which Context to use (SA1012)

Using a deprecated function, variable, constant or field (SA1019)

d6ffdbd

should not use built-in type string as key for value; define your own…

e6d8a74

… type to avoid collisions (SA1029)

removed unused code (U1000)

030d974

error strings should not be capitalized (ST1005)

4d895a9

don't use unit-specific suffix Seconds (ST1011)

64384eb

should use time.Since instead of time.Now().Sub (S1012)

f1350d4

should use time.Until instead of t.Sub(time.Now()) (S1024)

30bf124

this value of ... is never used (SA4006)

a1e55db

Fixing integration test

7158bb6

Fix TestDNSSRVMode

c00d8b6

Merge branch 'master' into staticcheck-cleanup

581a610

Fix TestExecutionFixerActivity_Success

952f9dd

Merge branch 'staticcheck-cleanup' of github.com:vytautas-karpavicius…

fd03d0d

…/cadence into staticcheck-cleanup

vytautas-karpavicius requested a review from a team February 24, 2022 14:22

vytautas-karpavicius marked this pull request as ready for review February 24, 2022 14:22

Groxx reviewed Feb 24, 2022

View reviewed changes

Groxx reviewed Feb 25, 2022

View reviewed changes

service/frontend/clusterRedirectionHandler_test.go Outdated Show resolved Hide resolved

Groxx reviewed Feb 25, 2022

View reviewed changes

Groxx approved these changes Feb 25, 2022

View reviewed changes

vytautas-karpavicius and others added 2 commits February 25, 2022 09:20

Use int64 for binary.Write

ce4e2cd

Update service/frontend/clusterRedirectionHandler_test.go

25e371f

Co-authored-by: Steven L <imgroxx@gmail.com>

vytautas-karpavicius merged commit 7084679 into cadence-workflow:master Feb 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Staticcheck cleanup #4751

Staticcheck cleanup #4751

vytautas-karpavicius commented Feb 24, 2022 •

edited

Loading

coveralls commented Feb 24, 2022 •

edited

Loading

Groxx Feb 24, 2022

vytautas-karpavicius Feb 25, 2022

Groxx Feb 24, 2022

Groxx Feb 24, 2022

vytautas-karpavicius Feb 25, 2022

Groxx Feb 24, 2022

Groxx Feb 24, 2022

vytautas-karpavicius Feb 25, 2022

Groxx Feb 24, 2022

vytautas-karpavicius Feb 25, 2022

Groxx Feb 24, 2022

vytautas-karpavicius Feb 25, 2022

Groxx Feb 24, 2022

Groxx Feb 24, 2022

vytautas-karpavicius Feb 25, 2022

Groxx Feb 24, 2022

Groxx Feb 24, 2022

vytautas-karpavicius Feb 25, 2022

Groxx Feb 24, 2022

Groxx Feb 24, 2022

Groxx Feb 24, 2022 •

edited

Loading

vytautas-karpavicius Feb 25, 2022

Groxx Feb 25, 2022 •

edited

Loading

vytautas-karpavicius Feb 25, 2022

Groxx Feb 25, 2022

Groxx Feb 25, 2022

vytautas-karpavicius Feb 25, 2022

Groxx Feb 25, 2022

vytautas-karpavicius Feb 25, 2022

Groxx commented Feb 25, 2022

		errEmptyDomainName = errors.New("Domain name is empty")
		errEmptyDomainName = errors.New("DomainName is empty")

		statter, err := statsd.NewBufferedClient(config.HostPort, config.Prefix, config.FlushInterval, config.FlushBytes)
		statter, err := statsd.NewClientWithConfig(&statsd.ClientConfig{

		binary.Write(buf, binary.LittleEndian, i)
		binary.Write(buf, binary.LittleEndian, int32(i))

		events = s.getHistory(s.domainName, &types.WorkflowExecution{
		s.getHistory(s.domainName, &types.WorkflowExecution{

		require.Error(t, errRemoteSyncMatchFailed) // should not persist the task
		require.Error(t, err) // should not persist the task

		@@ -43,6 +43,8 @@ import (

		const testWorkflowName = "default-test-workflow-type-name"

		var validBranchToken = []byte{89, 11, 0, 10, 0, 0, 0, 12, 116, 101, 115, 116, 45, 116, 114, 101, 101, 45, 105, 100, 11, 0, 20, 0, 0, 0, 14, 116, 101, 115, 116, 45, 98, 114, 97, 110, 99, 104, 45, 105, 100, 0}

Staticcheck cleanup #4751

Staticcheck cleanup #4751

Conversation

vytautas-karpavicius commented Feb 24, 2022 • edited Loading

coveralls commented Feb 24, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1f83e6c5-190c-4e80-aa95-78a09dcf5978

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Groxx Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Groxx Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Groxx commented Feb 25, 2022

vytautas-karpavicius commented Feb 24, 2022 •

edited

Loading

coveralls commented Feb 24, 2022 •

edited

Loading

Groxx Feb 24, 2022 •

edited

Loading

Groxx Feb 25, 2022 •

edited

Loading