feat: add golangci-lint #569

hhsnopek · 2022-10-17T22:10:36Z

Context

This pull request introduces golangci-lint into the existing CI/CD Pipeline to increase the project's code health. Applying go fmt to all files, removing blank identifier usage in all Wait Strategies for the implicit acceptance of the interface, removes deprecated functions/methods, and fixes a lot of missing error handling.

Additionally, in 9db062a I've exposed the noopStrategyTarget as waittest.NopStrategyTarget for others to consume in their own tests. It was renamed to match the Nop naming style used in Go's standard lib. This does not need to be apart of these changes and can be removed from this PR if required.

hhsnopek · 2022-10-18T04:29:16Z

Transitioning to a draft while I fix tests

parallel_test.go

.github/workflows/golangci-lint.yml

docker_test.go

mdelapenya · 2022-10-18T05:32:46Z

Thanks for this PR, it's fantastic that you took the time to solve all those issues. There is a PR adding commits to fix this: #552.

@baez90 I'd prefer this PR, once fixed the tests, instead of #552 as it includes the golanglint-ci support. Wdyt?

prskr · 2022-10-18T16:49:20Z

Sure! If possible it would be awesome if the gotest.tools/v3 reference could be replaced with standard library test helpers 😊 but apart from that this looks great! Always amazed if I see golangci-lint in action 😍

hhsnopek · 2022-10-22T03:31:31Z

@baez90 I've removed the dependency via refactor: remove gotest.tools/v3 usage

codecov · 2022-10-22T04:48:50Z

Codecov Report

Merging #569 (8a07783) into main (f3b2e5e) will increase coverage by 1.93%.
The diff coverage is 10.71%.

❗ Current head 8a07783 differs from pull request most recent head 3a426a1. Consider uploading reports for the commit 3a426a1 to get more accurate results

@@            Coverage Diff             @@
##             main     #569      +/-   ##
==========================================
+ Coverage   13.54%   15.48%   +1.93%     
==========================================
  Files          15       13       -2     
  Lines        2001     1770     -231     
==========================================
+ Hits          271      274       +3     
+ Misses       1684     1448     -236     
- Partials       46       48       +2

Impacted Files	Coverage Δ
compose.go	`0.00% <0.00%> (ø)`
docker.go	`20.41% <0.00%> (+0.16%)`	⬆️
file.go	`0.00% <0.00%> (ø)`
reaper.go	`3.44% <0.00%> (-0.07%)`	⬇️
testing.go	`15.00% <21.42%> (+15.00%)`	⬆️
compose_local.go
compose_api.go

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

testing.go

wait/exit.go

wait/health.go

wait/testdata/Dockerfile

docs/examples/nginx.md

docs/examples/cockroachdb.md

docs/features/creating_container.md

mdelapenya · 2022-11-08T06:36:38Z

wait/exec_test.go

@@ -29,7 +30,12 @@ func ExampleExecStrategy() {
 		panic(err)
 	}

-	defer localstack.Terminate(ctx) // nolint: errcheck
+	defer func() {
+		if err := localstack.Terminate(ctx); err != nil {


Wdyt about using testcontainers.Cleanup(t, ctx, container), not only here but everywhere, for consistency?

testcontainers.Cleanup shouldn't have been here. I agree it's a nice helper function, but it'll be moved to a separate PR.

hhsnopek · 2022-11-08T14:29:27Z

reaper.go

+			if _, err := sock.WriteString(strings.Join(labelFilters, "&")); err != nil {
+				continue
+			}
+
+			if _, err := sock.WriteString("\n"); err != nil {
+				continue
+			}


@mdelapenya Can you confirm this is okay? Tests pass, but I'd like to be extra careful here.

Given in https://github.com/testcontainers/testcontainers-go/pull/569/files#diff-e8ab4ec17e978db949cd15726ba0efa9c89fd714e7e6e0aa537ed7fc1bbde7fdR138 we continue on a flush failure, I'd say that it's okay

.github/workflows/golangci-lint.yml

wait/http_test.go

Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>

mdelapenya

LGTM, thanks for this! It will help decouple the main pipeline from the static analysis, where we can go adding more linters if/when needed.

At the same time, golangci-lint is becoming a de-facto standard on linting Go projects, so great addition!

Finally thanks for your support during the review, there were a lot of comments, but I think this PR is great now. Thank you! 👏

* main: feat: add golangci-lint (#569)

hhsnopek marked this pull request as ready for review October 17, 2022 22:10

hhsnopek requested a review from a team as a code owner October 17, 2022 22:10

hhsnopek force-pushed the hs/golangci-lint branch from a35232c to 1b9bab2 Compare October 18, 2022 04:28

hhsnopek marked this pull request as draft October 18, 2022 04:29

mdelapenya added the chore Changes that do not impact the existing functionality label Oct 18, 2022

mdelapenya reviewed Oct 18, 2022

View reviewed changes

parallel_test.go Show resolved Hide resolved

mdelapenya reviewed Oct 18, 2022

View reviewed changes

.github/workflows/golangci-lint.yml Outdated Show resolved Hide resolved

mdelapenya reviewed Oct 18, 2022

View reviewed changes

docker_test.go Outdated Show resolved Hide resolved

hhsnopek force-pushed the hs/golangci-lint branch 4 times, most recently from 52d6a8d to d57a76f Compare October 22, 2022 03:29

hhsnopek force-pushed the hs/golangci-lint branch 4 times, most recently from 7b09803 to e7e7d7f Compare October 22, 2022 03:38

hhsnopek marked this pull request as ready for review October 22, 2022 03:38

hhsnopek requested a review from mdelapenya October 22, 2022 03:38

hhsnopek force-pushed the hs/golangci-lint branch 2 times, most recently from dd09bfe to 7e25a6b Compare October 22, 2022 04:40

hhsnopek force-pushed the hs/golangci-lint branch from 7e25a6b to 304f44c Compare October 22, 2022 05:49

mdelapenya reviewed Oct 22, 2022

View reviewed changes

testing.go Outdated Show resolved Hide resolved

mdelapenya reviewed Oct 22, 2022

View reviewed changes

wait/exit.go Show resolved Hide resolved

mdelapenya reviewed Oct 22, 2022

View reviewed changes

wait/health.go Show resolved Hide resolved

mdelapenya reviewed Oct 22, 2022

View reviewed changes

wait/testdata/Dockerfile Show resolved Hide resolved

hhsnopek requested a review from mdelapenya November 8, 2022 05:15

hhsnopek added 3 commits November 7, 2022 23:17

feat: add golangci-lint

e105a7d

chore: go fmt -s .

09e49a6

chore: remove deprecated functions/methods

7dcdb92

hhsnopek force-pushed the hs/golangci-lint branch 2 times, most recently from b8c0ad8 to 0ec36f4 Compare November 8, 2022 05:52

mdelapenya reviewed Nov 8, 2022

View reviewed changes

hhsnopek force-pushed the hs/golangci-lint branch from 0ec36f4 to bea343a Compare November 8, 2022 14:25

hhsnopek added 2 commits November 8, 2022 08:28

chore: add missing error handling and remove unused variables

33213e1

remove: gotest.tools/v3 usage

f19472d

hhsnopek force-pushed the hs/golangci-lint branch from bea343a to f19472d Compare November 8, 2022 14:28

hhsnopek commented Nov 8, 2022

View reviewed changes

hhsnopek changed the title ~~[feat]: introduce golangci-lint~~ feat: add golangci-lint Nov 8, 2022

hhsnopek requested a review from mdelapenya November 8, 2022 14:37

mdelapenya reviewed Nov 9, 2022

View reviewed changes

.github/workflows/golangci-lint.yml Outdated Show resolved Hide resolved

mdelapenya reviewed Nov 9, 2022

View reviewed changes

wait/http_test.go Outdated Show resolved Hide resolved

mdelapenya reviewed Nov 9, 2022

View reviewed changes

wait/http_test.go Outdated Show resolved Hide resolved

hhsnopek added 3 commits November 9, 2022 10:02

review: path -> fileppath

3358132

review: rename golangci -> static-analysis

e6c664b

fix: static-analysis job

cf61983

mdelapenya reviewed Nov 9, 2022

View reviewed changes

wait/http_test.go Outdated Show resolved Hide resolved

review: remove leading slash in filepath.Join

3a426a1

Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>

hhsnopek requested a review from mdelapenya November 9, 2022 16:45

mdelapenya approved these changes Nov 9, 2022

View reviewed changes

mdelapenya merged commit 5432154 into testcontainers:main Nov 9, 2022

mdelapenya referenced this pull request in mdelapenya/testcontainers-go Nov 9, 2022

Merge branch 'main' into examples

ea9cff0

* main: feat: add golangci-lint (#569)

mdelapenya mentioned this pull request Nov 9, 2022

docs: fix syntax #615

Merged

mdelapenya self-assigned this Nov 10, 2022

This was referenced Nov 10, 2022

feat: expose terminateContainerOnEnd as Cleanup #616

Closed

feat: add golangci-lint testcontainers/moby-ryuk#48

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add golangci-lint #569

feat: add golangci-lint #569

hhsnopek commented Oct 17, 2022 •

edited

hhsnopek commented Oct 18, 2022

mdelapenya commented Oct 18, 2022

prskr commented Oct 18, 2022

hhsnopek commented Oct 22, 2022

codecov bot commented Oct 22, 2022 •

edited

mdelapenya Nov 8, 2022

hhsnopek Nov 8, 2022

hhsnopek Nov 8, 2022

mdelapenya Nov 9, 2022

mdelapenya left a comment

feat: add golangci-lint #569

feat: add golangci-lint #569

Conversation

hhsnopek commented Oct 17, 2022 • edited

Context

hhsnopek commented Oct 18, 2022

mdelapenya commented Oct 18, 2022

prskr commented Oct 18, 2022

hhsnopek commented Oct 22, 2022

codecov bot commented Oct 22, 2022 • edited

Codecov Report

mdelapenya Nov 8, 2022

Choose a reason for hiding this comment

hhsnopek Nov 8, 2022

Choose a reason for hiding this comment

hhsnopek Nov 8, 2022

Choose a reason for hiding this comment

mdelapenya Nov 9, 2022

Choose a reason for hiding this comment

mdelapenya left a comment

Choose a reason for hiding this comment

hhsnopek commented Oct 17, 2022 •

edited

codecov bot commented Oct 22, 2022 •

edited