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

ci: correct commitlint usage #504

Merged
merged 4 commits into from
Apr 5, 2024
Merged

ci: correct commitlint usage #504

merged 4 commits into from
Apr 5, 2024

Conversation

TobiTenno
Copy link
Collaborator

@TobiTenno TobiTenno commented Mar 22, 2024

@TobiTenno TobiTenno changed the title ci: commitlint correct usage ci: correct commitlint usage Mar 22, 2024
@TobiTenno
Copy link
Collaborator Author

this time i tested locally to make sure this substitution from the removed --last flag worked

@TobiTenno TobiTenno enabled auto-merge (squash) March 22, 2024 16:09
@TobiTenno
Copy link
Collaborator Author

@tukusejssirs @emagnier trying to fix up the last of what i broke, i've tested this a bit more on other repos since i last changed this

@tukusejssirs tukusejssirs added the concept: infrastructure Anything related to the surrounding tools and infrastructure to support romcal label Apr 4, 2024
@tukusejssirs tukusejssirs added this to the romcal v3.0 milestone Apr 4, 2024
@tukusejssirs
Copy link
Member

tukusejssirs commented Apr 4, 2024

I’m just curious: what was wrong with --last?

In commitlint --help (using @commitlint/cli@19.2.1), I can see that --last/-l is also possible:

-l, --last           just analyze the last commit; applies if edit=false

That said, -f HEAD~1 -t HEAD should work exactly the same way.

@TobiTenno
Copy link
Collaborator Author

TobiTenno commented Apr 4, 2024

@tukusejssirs
that's what I thought was supposed to work as well, but when i tested locally, --last didn't exist for me

it's also not listed on their CLI reference

@TobiTenno
Copy link
Collaborator Author

ope! looks like they just readded it

@TobiTenno
Copy link
Collaborator Author

i'll retest locally and see if i can sort it out, i'll add the commitlint version print as well

@TobiTenno
Copy link
Collaborator Author

looks like the one i had installed is old. i'll get that updated and re-use --last. thanks for the extra eyes, @tukusejssirs

@TobiTenno TobiTenno force-pushed the lint-again branch 4 times, most recently from ae062a0 to a442e9a Compare April 4, 2024 23:52
@TobiTenno
Copy link
Collaborator Author

@tukusejssirs let me know if you're satisfied by this current state.

to explain a bit, the prettier format -> dedupe -> sort would have to be in both the lint-staged cleanup and the dedupe on the check to ensure they both format out the same

@tukusejssirs
Copy link
Member

At first, I’ve thought you sorted package*.json file alphabetically, however, after reading the README.md file of sort-package-json, I know it does it in a different order. I personally usually sort JSON files alphabetically using jq -S, however, I don’t about this much (I always use features of my IDE or grep to find the properties/values I want.

However, now the PR name and description does not match the changes made. You seem to prettify and sort package*.json files in the CI after deduping them. After vhanging them, I see no reason not to commit this PR.

@TobiTenno TobiTenno merged commit 0c92112 into romcal:dev Apr 5, 2024
3 checks passed
@TobiTenno TobiTenno deleted the lint-again branch April 5, 2024 05:16
@tukusejssirs
Copy link
Member

Oh, and here the auto-merge goes: I wanted you change the PR desc, but not to slow you down by waiting on me to approve the changes … I have forgotten about the auto-merge being enabled.

@TobiTenno
Copy link
Collaborator Author

TobiTenno commented Apr 5, 2024

I'm happy to do another, but i'm not sure that I understand what you're asking. I change them in CI after deduping (as mentioned above) so that it's the same treatment they'd get from the commit hook to format -> dedupe -> sort so there'd be no git differences, which would fail the dedupe check

@TobiTenno
Copy link
Collaborator Author

TobiTenno commented Apr 5, 2024

I think the dedupe check (specifically the check, not the local part) just may not work out due to how it interferes with dependabot updates, and we have no way of forcing dependabot to dedupe

@tukusejssirs
Copy link
Member

I'm happy to do another, but i'm not sure that I understand what you're asking. I change them in CI after deduping (as mentioned above) so that it's the same treatment they'd get from the commit hook to format -> dedupe -> sort so there'd be no git differences, which would fail the dedupe check

Nothing to do anymore. I was suggesting to update the OP which could be used in the MR commit message which is not the case on GitHub when squashing the changes (0c92112).

Anyway, I still don’t really like the the auto-merge option being enabled, as whenever I (or someone else) approve a PR, it might still have some nitpicks which I raise without requiring them, thus I let the PR author decide whether they implement them or not. Sometimes another reviewer (e.g. Etienne) might disagree with me, however, the PR would be already merged, thus a revert would be potentially required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concept: infrastructure Anything related to the surrounding tools and infrastructure to support romcal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants