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

🌱 fix linting #6436

Merged
merged 1 commit into from
Apr 22, 2022
Merged

🌱 fix linting #6436

merged 1 commit into from
Apr 22, 2022

Conversation

apricote
Copy link
Member

What this PR does / why we need it:

Fix linting issues that are currently causing test failures in unrelated PRs. Issues were introduced by #6395, but I don't understand why they did not fail the checks back then.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 22, 2022
@schrej
Copy link
Member

schrej commented Apr 22, 2022

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 22, 2022
@stmcginnis
Copy link
Contributor

/lgtm

Thanks!

@jackfrancis
Copy link
Contributor

Thank you @apricote!

@enxebre
Copy link
Member

enxebre commented Apr 22, 2022

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2022
@k8s-ci-robot k8s-ci-robot merged commit 09e7790 into kubernetes-sigs:main Apr 22, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone Apr 22, 2022
@sbueringer
Copy link
Member

sbueringer commented Apr 26, 2022

@apricote @CecileRobertMichon The issue wasn't caught in #6395 because the golangci-lint action was not run on the final commit of the PR. This was probably caused by the issue that we have to manually allow checks to run after each commit on PRs of new contributors.

@CecileRobertMichon
Copy link
Contributor

@sbueringer this is because the github action (and same thing with other actions we run) is not marked as required on the main branch by branch protection rules in https://github.com/kubernetes-sigs/cluster-api/settings/branch_protection_rules

Do we want to mark this and other actions (if so which ones?) as required to make sure PRs can't merge without the action passing?

@sbueringer
Copy link
Member

I would suggest:
image

But I think the important ones are the linter actions if you prefer to not add the other ones

@CecileRobertMichon
Copy link
Contributor

CecileRobertMichon commented Apr 26, 2022

@sbueringer I made the first 4 required but PR Desc and PR Type don't seem to show up as their own status. Theoretically this should prevent tide from merging but looks like there might be a bug preventing this behavior from working as expected according to kubernetes/test-infra#24921.

kubernetes/test-infra#25210 should also help with this so we can run all checks when "/ok-to-test" is commented on the PR for a new contributor.

@CecileRobertMichon
Copy link
Contributor

I also found kubernetes/test-infra#22630 which seems to be doing what we want but the follow-up PRs suggest it didn't fully work... @cpanato @dims any insight on how we can make our lint GH Action required for tide merge in the CAPI repo? We want to make sure tide doesn't merge the PR before the action even runs when the contributor is new and the action needs manual approval before it can run.

@cpanato
Copy link
Member

cpanato commented Apr 27, 2022

I also found kubernetes/test-infra#22630 which seems to be doing what we want but the follow-up PRs suggest it didn't fully work... @cpanato @dims any insight on how we can make our lint GH Action required for tide merge in the CAPI repo? We want to make sure tide doesn't merge the PR before the action even runs when the contributor is new and the action needs manual approval before it can run.

afaik you just need to make that check required in the repository and then prow will merge if all required checks are passing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants