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

checkpatch: update image (skip backports, add a check, suppress one report type) #15096

Merged
merged 1 commit into from
Feb 27, 2021

Conversation

qmonnet
Copy link
Member

@qmonnet qmonnet commented Feb 24, 2021

Update the checkpatch image tag (and switch to quay.io) to benefit from the following changes:

  • Skip checkpatch validation on backport branches
  • Suppress reports of type COMMIT_LOG_LONG_LINE
  • Add a repo-wide check on commit subject length
  • Use $GITHUB_REPOSITORY variable when retrieving commits

For skipping backports we could instead update the conditions for running the job in the current repo, but that would be troublesome if we want to mark the action as required in the future. Therefore, we delegate the decision to the shell script in the checkpatch image: it returns early if a PR is not based on the 'master' branch.

Checkpatch updates available at https://github.com/cilium/image-tools/pull/104/files and https://github.com/cilium/image-tools/pull/109/files.

Tested here for the new check and the suppressed report, and there for skipping the backports.

Marking for backports for 1.9, where the checkpatch action is run but where we want to skip it.

No CI test needed, other than having the checkpatch verification still working.

@qmonnet qmonnet added area/CI Continuous Integration testing issue or flake release-note/ci This PR makes changes to the CI. needs-backport/1.9 labels Feb 24, 2021
@qmonnet qmonnet requested review from a team as code owners February 24, 2021 16:59
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 24, 2021
@qmonnet qmonnet requested a review from aanm February 24, 2021 16:59
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.5 Feb 24, 2021
.github/workflows/bpf-checks.yaml Outdated Show resolved Hide resolved
@qmonnet qmonnet requested a review from aanm February 24, 2021 17:50
@qmonnet qmonnet marked this pull request as draft February 25, 2021 00:07
@qmonnet
Copy link
Member Author

qmonnet commented Feb 25, 2021

I have been made aware of another issue to fix on the checkpatch image, I'll update the image once more and then update this PR.

@qmonnet qmonnet marked this pull request as ready for review February 25, 2021 00:41
@qmonnet
Copy link
Member Author

qmonnet commented Feb 25, 2021

Now ready for review again, I updated the test PRs too.

.github/workflows/bpf-checks.yaml Outdated Show resolved Hide resolved
@qmonnet qmonnet added dont-merge/blocked Another PR must be merged before this one. and removed dont-merge/blocked Another PR must be merged before this one. labels Feb 25, 2021
@qmonnet qmonnet force-pushed the pr/checkpatch_update branch 2 times, most recently from 52fd318 to f26ccea Compare February 25, 2021 15:51
Update the checkpatch image tag (and switch to quay.io) to benefit from
the following changes:

- Skip checkpatch validation on backport branches
- Suppress reports of type COMMIT_LOG_LONG_LINE
- Add a repo-wide check on commit subject length
- Use $GITHUB_REPOSITORY variable when retrieving commits

For skipping backports we could instead update the conditions for
running the job in the current repo, but that would be troublesome if we
want to mark the action as required in the future. Therefore, we
delegate the decision to the shell script in the checkpatch image: it
returns early if a PR is not based on the 'master' branch.

Signed-off-by: Quentin Monnet <quentin@isovalent.com>
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Do we also need to move this out of BPF checks given it now applies to all commits?

@pchaigno pchaigno removed their assignment Feb 26, 2021
@qmonnet
Copy link
Member Author

qmonnet commented Feb 26, 2021

Do we also need to move this out of BPF checks given it now applies to all commits?

We could. I don't know if it's worth it, there's only one check that extends to all commit, the majority is still for bpf/. Do you have a suggestion? I'd rather avoid creating a new action just for that.

@pchaigno
Copy link
Member

Do we also need to move this out of BPF checks given it now applies to all commits?

We could. I don't know if it's worth it, there's only one check that extends to all commit, the majority is still for bpf/. Do you have a suggestion? I'd rather avoid creating a new action just for that.

Yeah, we have too many actions already... Let's postpone.

@pchaigno pchaigno unassigned gandro and aanm Feb 26, 2021
@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 26, 2021
@fristonio fristonio merged commit 42fcb07 into cilium:master Feb 27, 2021
1.10.0 automation moved this from In progress to Done Feb 27, 2021
@qmonnet qmonnet deleted the pr/checkpatch_update branch March 1, 2021 09:42
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.5 Mar 2, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.5 Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
No open projects
1.9.5
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

None yet

7 participants