golangci-lint misconfiguration in GitHub actions

[ivanvc]

Bug report criteria

• This bug report is not security related, security issues should be disclosed privately via etcd maintainers.
• This is not a support request or question, support requests or questions should be raised in the etcd discussion forums.
• You have read the etcd bug reporting guidelines.
• Existing open issues along with etcd frequently asked questions have been checked and this is not a duplicate.

What happened?

As I've been working on enabling golangci linters (#17286, #17578), it came to my attention that when there's a linter violation, we're not getting the comments that the golangci-lint-action GitHub action highlights in the Files Changed tab.

What did you expect to happen?

Have the linter violations highlighted, as it provides helpful feedback for the contributor to address the issues before merging.

How can we reproduce it (as minimally and precisely as possible)?

I created a PR in my fork showing the current behavior:

1. PR: Current behavior: Force linter violation ivanvc/etcd#43
2. Files Changed tab: https://github.com/ivanvc/etcd/pull/43/files
3. The contributor needs to find the check that failed, and inspect it to understand the issue: https://github.com/ivanvc/etcd/actions/runs/8930220795/job/24529807300?pr=43

Anything else we need to know?

The reason why it's not working is because the GitHub workflow is misconfigured. This is the current configuration:

etcd/.github/workflows/static-analysis.yaml

Lines 17 to 21 in ce45881

	- name: golangci-lint
	uses: golangci/golangci-lint-action@9d1e0624a798bb64f6c3cea93db47765312263dc # v5.1.0
	with:
	version: ${{ steps.golangci_lint_version.outputs.golangci_lint_version }}
	args: --config tools/.golangci.yaml

Quoting golangci/golangci-lint-action's configuration documentation:

      # Optional: working directory, useful for monorepos
      # working-directory: somedir

This means that the current step tests only the root directory, and there are no violations, so it doesn't highlight them.

But why does it still work (i.e., it still fails on my build)? It's because later we call make verify:

etcd/.github/workflows/static-analysis.yaml

Lines 34 to 37 in ce45881

	- run: |
	set -euo pipefail
	make verify

This runs golangci-lint for each module (and again for the top-level directory).

Etcd version (please run commands below)

$ etcd --version
# paste output here

$ etcdctl version
# paste output here

Etcd configuration (command line flags or environment variables)

paste your configuration here

Etcd debug information (please run commands below, feel free to obfuscate the IP address or FQDN in the output)

$ etcdctl member list -w table
# paste output here

$ etcdctl --endpoints=<member list> endpoint status -w table
# paste output here

Relevant log output

No response

[ivanvc]

I can think of two possible solutions for the issue:

1. Keep golangci-lint-action

Properly configuring the action, would mean that we would need to generate a matrix as a previous step, so we can run golangci-lint for each module (without hardcoding the module).

• I created an example PR in my fork: Matrix approach: Test golangci action matrix approach ivanvc/etcd#44
• It shows the feedback in the Files Changed tab: https://github.com/ivanvc/etcd/pull/44/files#diff-c6650e0916ae5772325e4ff14f74f983c15cf0d94fb5ef74b852787e3936a933

Pro

• It keeps the configuration on how to lint each module at the GitHub action/workflow configuration file (see https://github.com/ivanvc/etcd/pull/44/files#diff-5d68f6ca696b457d708e1895ce1dcab8820f9d6d9357b3876176cbcdd9808b98)

Cons

• It fans out a job per module that we have, which crowds the runners more than what we have right now; see: https://github.com/ivanvc/etcd/actions/runs/8931512279
• When we later run make verify we run golangci-lint across all of the modules again

2. Add custom behavior in scripts/test_lib.sh

If we run make verify later in the workflow, we could update scripts/test_lib.sh to tell golangci-lint that the output format should be github-actions (so it provides the annotations in the Files Changed tab), and specify the prefix as the module directory.

• I created an example PR in my fork: Env variable approach: Test golangci action using an environment variable ivanvc/etcd#45
• It shows the feedback in the Files Changed tab: https://github.com/ivanvc/etcd/pull/45/files#diff-c6650e0916ae5772325e4ff14f74f983c15cf0d94fb5ef74b852787e3936a933

Pros

• It doesn't fan out a set of new runners, so it doesn't crowd the current actions
• It optimizes the execution time of the action as it adds the annotations in the same pass

Cons

• The changes to the script files are not pretty. We need the module's directory to properly annotate the files in GitHub (golangci-lint's --prefix-path option). The only way I can think of this is by dynamically receiving an unevaluated environment variable, i.e. ($MODULE_DIR), and then replacing it at execution time, refer to the patch:

diff --git a/scripts/test_lib.sh b/scripts/test_lib.sh
index b521a06ee..074028d3e 100644
--- a/scripts/test_lib.sh
+++ b/scripts/test_lib.sh
@@ -161,7 +161,7 @@ function run_for_module {
   local module=${1:-"."}
   shift 1
   (
-    cd "${ETCD_ROOT_DIR}/${module}" && "$@"
+    cd "${ETCD_ROOT_DIR}/${module}" && "${@//\$MODULE_DIR/$module}"
   )
 }

[ivanvc]

I feel like the best option would be the second one, but just laying out the possible solutions to get feedback.

cc. @jmhbnz, and maybe @ArkaSaha30. I'm not sure who else could provide feedback regarding this area.

[jmhbnz]

I feel like the best option would be the second one

Agree - My preference would be to focus on our pattern of make verify and do away with the standalone golangci-lint action usage.

The pr decoration/annotation would be nice for failures but personally I am not too worried about this. Expectation for contributors is they can run make verify locally and see output of any failures that need attention.

[ivanvc]

Sounds good, fair point. I was thinking of contributor experience, as I feel it's easier to see annotations on the PR. If we decide not to add the annotations, there's still some cleanup to do to avoid running twice golangci-lint.

[ArkaSaha30]

I feel like the best option would be the second one

I agree with the second option as well, since it is more github independent (since we need to run golangci-lint as a part of pull-etcd-verify prowjob).
PR ivanvc#45 seems to tackle the scenario of running on individual modules as well as top level.

[ivanvc]

Actually, if the ultimate intention is to migrate running golangci-lint to a Prow Job, then it doesn't make sense to address this issue, as we'll lose the feature in the future.

Is that the plan? Do we have a plan for what we're keeping as GitHub actions/workflows and what as Prow Jobs?

[jmhbnz]

Actually, if the ultimate intention is to migrate running golangci-lint to a Prow Job, then it doesn't make sense to address this issue, as we'll lose the feature in the future.

Is that the plan? Do we have a plan for what we're keeping as GitHub actions/workflows and what as Prow Jobs?

Yes. We want to migrate all etcd continuous integration to kubernetes project community test infrastructure. I suggest we just remove the unnecessary github action usage for golangci-lint under this issue and stick with make verify which is already being called by our prow job for static analysis https://github.com/kubernetes/test-infra/blob/master/config/jobs/etcd/etcd-presubmits.yaml#L53-L86

[ivanvc]

Actually, if the ultimate intention is to migrate running golangci-lint to a Prow Job, then it doesn't make sense to address this issue, as we'll lose the feature in the future.
Is that the plan? Do we have a plan for what we're keeping as GitHub actions/workflows and what as Prow Jobs?

Yes. We want to migrate all etcd continuous integration to kubernetes project community test infrastructure. I suggest we just remove the unnecessary github action usage for golangci-lint under this issue and stick with make verify which is already being called by our prow job for static analysis https://github.com/kubernetes/test-infra/blob/master/config/jobs/etcd/etcd-presubmits.yaml#L53-L86

In that case, doesn't it make more sense to remove the GitHub action?

[ivanvc]

As I was exploring removing the GitHub action, I realized that the Prow job is failing (I've already addressed why it's failing), and it's not correctly reporting that it has a failure status. In the runs, it always ends successfully despite an error.

Refer to a sample build: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/17934/pull-etcd-verify/1786627012845113344
It shows its status as:

Test started today at 10:20 PM passed after 26s. (less info)

I reviewed its configuration, and it's not set as optional. I also tried changing the exit status from 255 to 1. See #17935 / https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/17935/pull-etcd-verify/1786631288405364736.

Do you know if I'm missing something obvious? Or should I contact sig-testing about this issue?

[jmhbnz]

I reviewed its configuration, and it's not set as optional. I also tried changing the exit status from 255 to 1. See #17935 / https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/17935/pull-etcd-verify/1786631288405364736.

Do you know if I'm missing something obvious? Or should I contact sig-testing about this issue?

I have an idea why it's not reporting failure properly, take a look at: kubernetes/test-infra#32563

If you think it's worth trying please give it an /lgtm thanks 🙏🏻

[ivanvc]

Thanks @jmhbnz, it works: #17935 / https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/etcd-io_etcd/17935/pull-etcd-verify/1787233596726054912.

I think we can safely delete the GitHub workflow now.

[jmhbnz]

I think we can safely delete the GitHub workflow now.

Agreed - Feel free to raise it if you have time, thanks 🙏🏻

[ivanvc]

@jmhbnz, this ticket is morphing into replacing the static analysis GitHub workflow with a Prow job. There are more follow-up items. However, I feel the discussion is beyond the scope of the original issue. Should we continue the discussion here, or should I open a new issue to track the progress and close this one?

[jmhbnz]

@jmhbnz, this ticket is morphing into replacing the static analysis GitHub workflow with a Prow job. There are more follow-up items. However, I feel the discussion is beyond the scope of the original issue. Should we continue the discussion here, or should I open a new issue to track the progress and close this one?

Sorry I might not have been clear, I think we can delete https://github.com/etcd-io/etcd/blob/main/.github/workflows/static-analysis.yaml#L15-L21 but I don't think we should be removing the whole workflow yet. Just the unnecessary step for running standalone golangci-lint.

I think we can make that change under this issue, agree anything further should be a separate discussion.