chore(ci): run lint in a separate build before running the tests #2876

mdelapenya · 2024-11-05T16:55:34Z

What does this PR do?

It extracts the golangci-lint step to a separate job, so that it can be called separately before the tests.

To detect the changes, we have created a shell script, scripts/changed-modules.sh. This script receives one single env var as input parameter, ALL_CHANGED_FILES, which on CI will be provided by a Github action, https://github.com/tj-actions/changed-files, that puts in there a list of the modified files, comparing the current PR changeset with the parent branch (main). We can tune this up to always check the latest main branch (open to discussion here).

The script will compare all existing modules (looking up all the go.mod files of interest, no test files) with the modified files, building an array of modified modules. At the moment there is modified file in the core, in other words the entire root directory without modulegen/modules/examples, all the modules will be included in the build. There are some exclusions for the code: files in the .devcontainer, .vscode, docs, scripts will not cause all modules will be included. So it could be the case that no modules could end up in the build if only files in those exclusions are changed. In that case, we are going to conditionally skip the lint, test and upload-to-sonar jobs.

Finally, for running the tests, we are going to keep the matrix for running them in multiple Go versions and OSs, with the following rules:

any Go module will run the tests for the two most recent Go releases (core, modulegen, modules and examples).
the core module, modules and examples will run the tests on ubuntu-latest.
the modulegen module will run the tests on ubuntu-latest, macos-latest and windows-latest.
the jobs for reaper-off and rootless-mode will run the tests for the two most recent Go releases, on `ubuntu-latest, only.

BTW I was able to iterate faster on this at the moment I discovered the power of https://github.com/nektos/act. Fantastic tool!

Why is it important?

There are two major goals:

Do not run the lint until it's too late in the build.
Reduce the feedback loop when contributing to modules. At the moment, a module contributor has to wait for all the core changes to be built and pass the tests. With this approach, if the PR touches just one module, then that module will be added to the workflow.

As a drawback, in my opinion, is that two different builds with two different changesets won't run the same set of tests, so they are not deterministic. I think this is a minor detail that would be satisfied with the improved experience for module contributors at CI time. For core maintainers, we will probably run all the modules per PR, but that could be expected to verify we are not breaking the downstream modules.

How to test this PR

The shell script is the key part, and it's explained in the comments for the script. It requires running the script with the required environment variables for the different use cases:

A Go file from the core module is modified:
ALL_CHANGED_FILES="examples/nginx/go.mod examples/foo/a.txt a/b/c/d/a.go" ./scripts/changed-modules.sh
The output should be: all modules.
A file from a module in the modules dir is modified:
ALL_CHANGED_FILES="modules/nginx/go.mod" ./scripts/changed-modules.sh
The output should be: just the modules/nginx module.
A file from a module in the examples dir is modified:
ALL_CHANGED_FILES="examples/nginx/go.mod" ./scripts/changed-modules.sh
The output should be: just the examples/nginx module.
A Go file from the modulegen dir is modified:
ALL_CHANGED_FILES="modulegen/a.go" ./scripts/changed-modules.sh
The output should be: just the modulegen module.
A non-Go file from the core dir is modified:
ALL_CHANGED_FILES="README.md" ./scripts/changed-modules.sh
The output should be: all modules.
A file from two modules in the modules dir are modified:
ALL_CHANGED_FILES="modules/nginx/go.mod modules/localstack/go.mod" ./scripts/changed-modules.sh
The output should be: the modules/nginx and modules/localstack modules.
Files from the excluded dirs are modified:
ALL_CHANGED_FILES="docs/a.md .vscode/a.json .devcontainer/a.json scripts/a.sh" ./scripts/changed-modules.sh
The output should be: no modules.

Follow-ups

IMPORTANT!: We need to update the branch settings so that the new checks are used to allow the merge.

netlify · 2024-11-05T16:55:50Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`7684c55`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/67a24e116ee5170008ce866d
😎 Deploy Preview	https://deploy-preview-2876--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

This reverts commit 4bab0fd.

This reverts commit c9c63bd.

… based on that

…t instead

This reverts commit e30d9a0.

Editing the changed-modules script needs to run all

mdelapenya · 2025-02-03T11:14:54Z

@stevenh I'm tempted to merge this one. Do you want to take a look first?

stevenh

A few suggestions / questions

scripts/changed-modules.sh

stevenh · 2025-02-04T11:35:45Z

.github/workflows/ci-lint-go.yml

+          args: --verbose
+          # Optional: if set to true then the all caching functionality will be complete disabled,
+          #           takes precedence over all other caching options.
+          skip-cache: true


question: why set this to true?

This is already set to true in the main branch 🤔

I suspect it might be hang over from quite a few versions back where actions/setup-go and golangci/golangci-lint-action both used to cache package and build, but this was fixed in v4.

I would suggest removing it as it can have a significant impact on the time taken to run the linter, especially as we've been enabling more linting recently.

.github/workflows/ci-lint-go.yml

…rrors

stevenh

Clarified why I think skip-cache was likely set and suggest we remove for now, thoughts?

stevenh · 2025-02-04T14:32:35Z

.github/workflows/ci-lint-go.yml

+          args: --verbose
+          # Optional: if set to true then the all caching functionality will be complete disabled,
+          #           takes precedence over all other caching options.
+          skip-cache: true


I suspect it might be hang over from quite a few versions back where actions/setup-go and golangci/golangci-lint-action both used to cache package and build, but this was fixed in v4.

I would suggest removing it as it can have a significant impact on the time taken to run the linter, especially as we've been enabling more linting recently.

stevenh · 2025-02-04T14:48:22Z

Not sure if the expected test that's pending is a side effect of this change @mdelapenya?

mdelapenya · 2025-02-04T15:00:42Z

Not sure if the expected test that's pending is a side effect of this change @mdelapenya?

This is because we have a setting in the GH repo, as code in the .github/settings.yml file, with the minimal set of checks that are required to mark the PR as mergeable.

With a dynamic set of checks, I'm not sure if we need this setting anymore, or at least, look for a final node in the pipeline that is green. Do you agree that we could skip it and find the solution in a follow-up?

stevenh · 2025-02-04T17:26:16Z

You can remove it from the required check for now, but should follow up.

stevenh

We can follow with require checks changes if needed.

mdelapenya · 2025-02-05T09:18:52Z

I created #2968 to track the GH checks revamp

* main: (54 commits) deps: update go version from 1.22.0 to 1.23.0 (testcontainers#2985) feat(redpanda): add bootstrap user account option (testcontainers#2975) chore(ollama): bump default version to 0.5.7 (testcontainers#2966) feat!: log package for consistent output (testcontainers#2979) docs: remove duplicated options in the customisers lists (testcontainers#2989) chore: exclude "modules/k6" from the build (testcontainers#2987) chore: enable var-naming from revive (private vars only) (testcontainers#2978) chore(deps): bump actions/checkout from 4.1.7 to 4.2.2 (testcontainers#2971) chore(deps): bump release-drafter/release-drafter from 6.0.0 to 6.1.0 (testcontainers#2970) chore!: remove variadic arguments from nats ConnectionString (testcontainers#2967) fix(ci): use same condition for sonar steps (testcontainers#2974) fix: return unique modified modules (testcontainers#2973) chore(deps): bump golangci/golangci-lint-action from 6.2.0 to 6.3.0 (testcontainers#2969) chore(ci): run lint in a separate build before running the tests (testcontainers#2876) fix(deps): update to github.com/shirou/gopsutil/v4 (testcontainers#2964) fix(valkey): fix port race (testcontainers#2962) chore(deps): bump golang.org/x/net in /modules/pinecone (testcontainers#2963) chore(deps): bump golang.org/x/net from 0.26.0 to 0.33.0 (testcontainers#2961) deps(fix): include modulegen templates dir in dependabot updates (testcontainers#2956) chore(deps): bump docker/setup-docker-action from 4.0.0 to 4.1.0 (testcontainers#2959) ...

* main: (34 commits) deps: update go version from 1.22.0 to 1.23.0 (testcontainers#2985) feat(redpanda): add bootstrap user account option (testcontainers#2975) chore(ollama): bump default version to 0.5.7 (testcontainers#2966) feat!: log package for consistent output (testcontainers#2979) docs: remove duplicated options in the customisers lists (testcontainers#2989) chore: exclude "modules/k6" from the build (testcontainers#2987) chore: enable var-naming from revive (private vars only) (testcontainers#2978) chore(deps): bump actions/checkout from 4.1.7 to 4.2.2 (testcontainers#2971) chore(deps): bump release-drafter/release-drafter from 6.0.0 to 6.1.0 (testcontainers#2970) chore!: remove variadic arguments from nats ConnectionString (testcontainers#2967) fix(ci): use same condition for sonar steps (testcontainers#2974) fix: return unique modified modules (testcontainers#2973) chore(deps): bump golangci/golangci-lint-action from 6.2.0 to 6.3.0 (testcontainers#2969) chore(ci): run lint in a separate build before running the tests (testcontainers#2876) fix(deps): update to github.com/shirou/gopsutil/v4 (testcontainers#2964) fix(valkey): fix port race (testcontainers#2962) chore(deps): bump golang.org/x/net in /modules/pinecone (testcontainers#2963) chore(deps): bump golang.org/x/net from 0.26.0 to 0.33.0 (testcontainers#2961) deps(fix): include modulegen templates dir in dependabot updates (testcontainers#2956) chore(deps): bump docker/setup-docker-action from 4.0.0 to 4.1.0 (testcontainers#2959) ...

chore: run lint in a separate build before running the tests

c9c63bd

mdelapenya added the chore Changes that do not impact the existing functionality label Nov 5, 2024

mdelapenya self-assigned this Nov 5, 2024

mdelapenya requested a review from stevenh November 5, 2024 16:55

mdelapenya added 22 commits November 5, 2024 19:30

fix: lint work dir

4bab0fd

Revert "fix: lint work dir"

2f6d74e

This reverts commit 4bab0fd.

Revert "chore: run lint in a separate build before running the tests"

77b8c25

This reverts commit c9c63bd.

chore: detect modified files and select which module to run the tests…

eaef8e1

… based on that

fix: read env var properly

cfcec39

fix: simplify

4a4482f

fix: single quotes

6550d8e

chore: remove job

93934ac

fix: proper all_modified_files quotes

2e35ae8

chore: print out modules to run

87d9718

fix: do not send an array with all modules, but a comma-separated lis…

4b63312

…t instead

fix: do not prepend the root_dir for the modules

1c17f59

chore: proper runner definition

f68e6af

fix: use runner in the right place

3272c43

chore: separate lint to a different workflow

5e1f13a

fix: use fromJson

8a51fe6

fix: proper jobs chain

e9e62cc

fix: no need to lint twice

d0cf9ab

fix: remove non-existing input

afedcd6

fix: sanitise path

2352d19

chore: only run the reaper if the core module is modified

ff1a5cf

revert: simple lint

e30d9a0

mdelapenya force-pushed the run-lint-first branch from 2fc8641 to e30d9a0 Compare November 7, 2024 13:39

mdelapenya added 2 commits November 7, 2024 15:55

Revert "revert: simple lint"

84d2fd9

This reverts commit e30d9a0.

revet: remove lint

d66f266

mdelapenya added 2 commits February 3, 2025 09:42

chore: no need to read modulegen/_template in dependabot

6cb373e

chore: scripts is not excluded

ea94574

Editing the changed-modules script needs to run all

stevenh requested changes Feb 4, 2025

View reviewed changes

mdelapenya added 9 commits February 4, 2025 12:58

chore: exit on error, unset variables, print commands, fail on pipe e…

2af692e

…rrors

chore: remove verbosity in golangci-lint in the GH workflow

118327a

chore: use pinned version with 3 digits

ea6dfbb

chore: bump golangci-lint version to match makefile's

2128811

chore: omit default value for continue-on-error

674653a

chore: simplify removing the continue-on-error input

7971ae2

chore: bump golangci-lint GH action version

e0c395f

chore: do not run mod verify on new GH workers

3617369

fix: do not fail-fast modules

ec4302e

mdelapenya requested a review from stevenh February 4, 2025 14:04

stevenh requested changes Feb 4, 2025

View reviewed changes

chore: do not skip cache for golangci-lint GH action

3368afd

stevenh approved these changes Feb 4, 2025

View reviewed changes

Merge branch 'main' into run-lint-first

7684c55

mdelapenya mentioned this pull request Feb 5, 2025

Rethink the required GH checks #2968

Closed

mdelapenya merged commit d4b1a10 into testcontainers:main Feb 5, 2025
181 checks passed

mdelapenya deleted the run-lint-first branch February 5, 2025 09:24

This was referenced Feb 5, 2025

[Bug]: identify unique modules in the GH workflow #2972

Closed

fix: return unique modified modules #2973

Merged

fix(ci): use same condition for sonar steps #2974

Merged

Uh oh!

chore(ci): run lint in a separate build before running the tests #2876

chore(ci): run lint in a separate build before running the tests #2876

Uh oh!

Conversation

mdelapenya commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

How to test this PR

Follow-ups

Uh oh!

netlify bot commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for testcontainers-go ready!

Uh oh!

mdelapenya commented Feb 3, 2025

Uh oh!

stevenh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stevenh Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

mdelapenya Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

stevenh Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stevenh left a comment

Choose a reason for hiding this comment

Uh oh!

stevenh Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

stevenh commented Feb 4, 2025

Uh oh!

mdelapenya commented Feb 4, 2025

Uh oh!

stevenh commented Feb 4, 2025

Uh oh!

stevenh left a comment

Choose a reason for hiding this comment

Uh oh!

mdelapenya commented Feb 5, 2025

Uh oh!

Uh oh!

Uh oh!

mdelapenya commented Nov 5, 2024 •

edited

Loading

netlify bot commented Nov 5, 2024 •

edited

Loading