Skip to content

Conversation

@qdm12
Copy link

@qdm12 qdm12 commented Jan 2, 2025

Why this should be merged

Unblock the CI and update the copyright

How this works

Bumped the year

How this was tested

Lint job passing

@qdm12 qdm12 marked this pull request as ready for review January 2, 2025 16:46
@qdm12 qdm12 requested a review from ARR4N January 2, 2025 16:48
Copy link

@darioush darioush left a comment

Choose a reason for hiding this comment

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

should the CI check only apply to modified files? Seems odd to have to update files just for this.

@ARR4N
Copy link
Collaborator

ARR4N commented Jan 2, 2025

should the CI check only apply to modified files? Seems odd to have to update files just for this.

I agree. We should use this option.

@qdm12 qdm12 force-pushed the qdm12/copyright-2025 branch from 399d5e6 to f61df6a Compare January 3, 2025 08:46
@qdm12
Copy link
Author

qdm12 commented Jan 3, 2025

Ideally we should run only goheader on new files, since an upgrade to golangci-lint might find other lint errors in non-new files. I've created golangci/golangci-lint-action#1140 for this.

In the meantime, let's set only-new-issues: true in our golangci-lint action configuration (pushed on this branch).

PS: side thing I found, added in #88: Use golangci-lint's option new-from-rev: HEAD where HEAD is set automatically to the base fork commit - that only checks diffs on changed files compared to that commit, pretty cool.

@qdm12 qdm12 force-pushed the qdm12/copyright-2025 branch 2 times, most recently from 2a70c80 to 6c43849 Compare January 3, 2025 09:00
@qdm12 qdm12 changed the title chore(all): bump copyright notice from 2024 to 2025 chore(ci): linting only reports new issues from new files Jan 3, 2025
Copy link
Collaborator

@ARR4N ARR4N left a comment

Choose a reason for hiding this comment

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

Summary of changes, per existing discussion plus separately with legal:

  • Only flag changed files
  • Change the template to require the first year the file was modified and, if not this year, a range to now. I think I tried to do that in the beginning but MOD-YEAR-RANGE doesn't seem to play nice when it's not actually a range.

@qdm12 qdm12 force-pushed the qdm12/copyright-2025 branch from d33c6ae to 4ddf49f Compare January 6, 2025 16:31
@qdm12
Copy link
Author

qdm12 commented Jan 6, 2025

MOD-YEAR-RANGE doesn't seem to play nice when it's not actually a range.

It works fine for me, it fails with error Pattern ((20\d\d\-2025)|(2025)) doesn't match which makes sense. However, since we would ideally like to keep unmodified files to their last modified year, this is problematic (last modified should stay in 2024 or 20xx-2024). The Ci does the trick by only running on modified and new files, but running golangci-lint run locally would fail on old and not-modified files. To go around this, I decided to disable goheader locally and enable it only in the CI... deal??

Obviously the perfect solution would be a linter handling the git plumbing to find only changed files and scan headers for only those files, one can only dream 🤷 Unless it exists let me know!

@qdm12 qdm12 requested review from ARR4N and darioush January 6, 2025 16:48
@qdm12 qdm12 force-pushed the qdm12/copyright-2025 branch from 4ddf49f to 2833156 Compare January 6, 2025 16:52
@qdm12 qdm12 changed the title chore(ci): linting only reports new issues from new files chore(ci): goheader linting only runs in CI on changed or new files Jan 7, 2025
@qdm12 qdm12 enabled auto-merge (squash) January 7, 2025 19:37
Copy link
Collaborator

@ARR4N ARR4N left a comment

Choose a reason for hiding this comment

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

Approving with minor changes requested.

with:
version: v1.60
only-new-issues: true
args: --enable goheader
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment explaining why this is "manually" enabled via flag instead of in the config.

- gocheckcompilerdirectives
- gofmt
- goheader
- goimports
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add either:

  1. A comment where goheader would have been, pointing readers to the workflow config; or
  2. Copy the comment from the workflow config to here (with modification if necessary).

(1) feels better but I'm ok with the redundancy of 2 because this isn't depended on by any code / requires devs to read it regularly.

@qdm12 qdm12 merged commit efa14ab into main Jan 8, 2025
4 checks passed
@qdm12 qdm12 deleted the qdm12/copyright-2025 branch January 8, 2025 08:38
@qdm12
Copy link
Author

qdm12 commented Jan 8, 2025

@ARR4N Whoops I think that got auto-merged on your approval, I did #101 with your suggestions 👍

PS: we should be careful about approving & auto-merge enabled.
I used auto-merge because playing cat-and-mouse between keeping the branch up to date, waiting for Ci to complete and re-asking for approvals... is not too fun 🤷 Although after seeing this, perhaps we should disable auto-merge? The best thing to do in my view is to disable the requirement to be based on the last commit of the default branch, but to each their opinion 😄

@qdm12 qdm12 mentioned this pull request Feb 4, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants