Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add golangci-lint #569

Merged
merged 9 commits into from Nov 9, 2022
Merged

Conversation

hhsnopek
Copy link
Contributor

@hhsnopek hhsnopek commented Oct 17, 2022

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 hhsnopek marked this pull request as ready for review October 17, 2022 22:10
@hhsnopek hhsnopek requested a review from a team as a code owner October 17, 2022 22:10
@hhsnopek
Copy link
Contributor Author

Transitioning to a draft while I fix tests

@hhsnopek hhsnopek marked this pull request as draft October 18, 2022 04:29
@mdelapenya mdelapenya added the chore Changes that do not impact the existing functionality label Oct 18, 2022
parallel_test.go Show resolved Hide resolved
docker_test.go Outdated Show resolved Hide resolved
@mdelapenya
Copy link
Collaborator

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?

@baez90
Copy link
Contributor

baez90 commented Oct 18, 2022

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 hhsnopek force-pushed the hs/golangci-lint branch 4 times, most recently from 52d6a8d to d57a76f Compare October 22, 2022 03:29
@hhsnopek
Copy link
Contributor Author

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

@hhsnopek hhsnopek force-pushed the hs/golangci-lint branch 4 times, most recently from 7b09803 to e7e7d7f Compare October 22, 2022 03:38
@hhsnopek hhsnopek marked this pull request as ready for review October 22, 2022 03:38
@hhsnopek hhsnopek force-pushed the hs/golangci-lint branch 2 times, most recently from dd09bfe to 7e25a6b Compare October 22, 2022 04:40
@codecov
Copy link

codecov bot commented Oct 22, 2022

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 Outdated Show resolved Hide resolved
wait/exit.go Show resolved Hide resolved
wait/health.go Show resolved Hide resolved
@hhsnopek hhsnopek force-pushed the hs/golangci-lint branch 2 times, most recently from b8c0ad8 to 0ec36f4 Compare November 8, 2022 05:52
docs/examples/nginx.md Outdated Show resolved Hide resolved
docs/examples/cockroachdb.md Show resolved Hide resolved
docs/features/creating_container.md Show resolved Hide resolved
@@ -29,7 +30,12 @@ func ExampleExecStrategy() {
panic(err)
}

defer localstack.Terminate(ctx) // nolint: errcheck
defer func() {
if err := localstack.Terminate(ctx); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

if _, err := sock.WriteString("\n"); err != nil {
continue
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hhsnopek hhsnopek changed the title [feat]: introduce golangci-lint feat: add golangci-lint Nov 8, 2022
wait/http_test.go Outdated Show resolved Hide resolved
wait/http_test.go Outdated Show resolved Hide resolved
wait/http_test.go Outdated Show resolved Hide resolved
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 👏

@mdelapenya mdelapenya merged commit 5432154 into testcontainers:main Nov 9, 2022
14 checks passed
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this pull request Nov 9, 2022
* main:
  feat: add golangci-lint (testcontainers#569)
@mdelapenya mdelapenya mentioned this pull request Nov 9, 2022
@mdelapenya mdelapenya self-assigned this Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Changes that do not impact the existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants