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

Run golangci-lint as soon as ok-to-test is set #9215

Closed
sbueringer opened this issue Aug 16, 2023 · 19 comments · Fixed by #9244 or #9259
Closed

Run golangci-lint as soon as ok-to-test is set #9215

sbueringer opened this issue Aug 16, 2023 · 19 comments · Fixed by #9244 or #9259
Labels
area/devtools Issues or PRs related to devtools triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@sbueringer
Copy link
Member

sbueringer commented Aug 16, 2023

Today we have significant toil for maintainers because they always have to manually approve the golangci-lint action for outside contributors (by clicking on approve in the "checks box").

Goal of this issue is to ensure the action is run automatically as soon as the ok-to-test label is applied.

Proposed solution:

  1. Configure in the repo settings that actions only require approval for first time GitHub users
  2. Update the action to immediately fail with a nice error message if the ok-to-test label is missing
  • Prior art: contains(github.event.pull_request.labels.*.name, 'ok-to-test')

Probably needs a bit of trial-and-error on a fork, but I think this should work

For 1.
image

There is potentially another alternative, which is approve workflows automatically if ok-to-test is set: https://github.com/kubernetes-sigs/metrics-server/blob/19802fd979b3c8cdf23297cebe9dc62448e9dc9d/.github/workflows/gh-workflow-approve.yaml#L14. This seems somewhat okay as it only requires actions write permissions. But I think I would prefer the option I mentioned above

@sbueringer sbueringer added the area/devtools Issues or PRs related to devtools label Aug 16, 2023
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Aug 16, 2023
@sbueringer
Copy link
Member Author

@sbueringer
Copy link
Member Author

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 16, 2023
@sbueringer
Copy link
Member Author

Kudos to @rikatz for sharing the idea!!

@killianmuldoon
Copy link
Contributor

Sounds like a good idea to me for sure.

@chrischdi
Copy link
Member

That sounds great. And should be safe because gh actions do not run with the config which is on a PR.

@chrischdi
Copy link
Member

Solution 1 has an issue:

When e.g. adding the following to the steps of an action:

...
    steps:
      - name: Block testing
        if: ! contains(github.event.pull_request.labels.*.name, 'ok-to-test')
        run: echo "Requires ok-to-test" && exit 1

It would also require the ok-to-test label for PRs created by kubernetes-sigs members (which is currently not the case).

Also wondering how the second one works.

Using a gh action/workflow to allow gh actions/workflow. Isn't it the case that the gh-workflow-approve workflow won't run either?

@sbueringer
Copy link
Member Author

sbueringer commented Aug 17, 2023

Good points. What about checking for needs-ok-to-test?

@chrischdi
Copy link
Member

needs-ok-to-test

Nope, can't do that. Sounds too easy 😂 I'll give it a try. That should do it!

@chrischdi
Copy link
Member

Issue with that is: there is a small time window between creating a PR and gh actions which would run 🤔

@sbueringer
Copy link
Member Author

sbueringer commented Aug 17, 2023

Hm good point. Maybe you can check via https://cs.k8s.io/?q=%27ok-to-test%27&i=nope&files=&excludeFiles=&repos= how it works in other repos (and maybe also check how Prow determines when ok-to-test is needed)

@sbueringer
Copy link
Member Author

sbueringer commented Aug 17, 2023

Also wondering how the second one works.

I suspect it works somehow btw. Otherwise they use a broken action :D Maybe we just have to figure out how (or ask them)

@chrischdi
Copy link
Member

Jep, the GH action to approve the workflows when ok-to-test label exists works 👍 I'll create a PR for this.

@sbueringer
Copy link
Member Author

Sounds like a good option. Thx!

@killianmuldoon
Copy link
Contributor

Hmm - on this PR: #9251

The ok-to-test seemed to work initially to run the golangci-lint. But since the PR was pushed again it does not seem to be working - the workflow needs to be approved again.

/reopen

@k8s-ci-robot k8s-ci-robot reopened this Aug 21, 2023
@k8s-ci-robot
Copy link
Contributor

@killianmuldoon: Reopened this issue.

In response to this:

Hmm - on this PR: #9251

The ok-to-test seemed to work initially to run the golangci-lint. But since the PR was pushed again it does not seem to be working - the workflow needs to be approved again.

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sbueringer
Copy link
Member Author

Yup. It has to be approved after every single push

@sbueringer
Copy link
Member Author

@chrischdi
Copy link
Member

🤔 I'll test that again.

@chrischdi
Copy link
Member

Tricky to catch the issue but this fixes it: #9259

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues or PRs related to devtools triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
4 participants