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 the no-commit-to-branch check only locally #2913

Merged
merged 1 commit into from May 9, 2024
Merged

Conversation

psss
Copy link
Collaborator

@psss psss commented May 7, 2024

It does not make sense to run this pre-commit check in pull requests or when merging to the main branch on GitHub. Also update actions to the latest versions to get rid of warnings.

@psss psss added this to the 1.34 milestone May 7, 2024
@lukaszachy
Copy link
Collaborator

I got confused by "It does not make sense to run it when merging to main." as almost every PR is aiming to be merged into main. But as gh runs action after it is merged then OK to disable it.

BTW summary and PR doesn't match - this PR disables whole pre-commit, not just one hook, right? Is there a way to disable just this check?

@psss
Copy link
Collaborator Author

psss commented May 7, 2024

Is there a way to disable just this check?

Hm, don't know if it's possible to only disable a specific check. @thrix, any hint?

BTW summary and PR doesn't match - this PR disables whole pre-commit, not just one hook, right?

Ah, right, will update the title if we keep it as it is.

@happz
Copy link
Collaborator

happz commented May 7, 2024

Interestingly, in the Artemis project, pre-commit runs after merge event, against main, and it reports no problems with main branch... I'm puzzled :/

@psss
Copy link
Collaborator Author

psss commented May 7, 2024

Interestingly, in the Artemis project, pre-commit runs after merge event, against main, and it reports no problems with main branch... I'm puzzled :/

Hmmm, that's really weird.

@psss An attempt to disable single hook.

Great! Thanks for preparing this. Tried with 8366c74 and confirm it works nicely! Let's approve and merge soon!

@psss psss force-pushed the no-commit-to-branch branch 2 times, most recently from 7b634aa to 055aa25 Compare May 7, 2024 20:46
@psss psss added the process label May 7, 2024
@LecrisUT
Copy link
Contributor

LecrisUT commented May 7, 2024

Doing a code search. Examples with --branch seem to be using pre-commit-ci. Without it the default is to use master and main, qnd I see it used with the pre-commit action without erroring 🤷‍♂️

@happz
Copy link
Collaborator

happz commented May 8, 2024

Nope, no idea why it works in Artemis and fails here. no-commit-to-banch is apparently executed against main at https://gitlab.com/testing-farm/artemis/-/jobs/6784553461#L435, and it's green and happy. The only idea I have is that it's not checking any new commit, it's running against main that's been already enriched with new commits, it's done and no changes are pending to be committed to main. Is that different than Github actions we saw fail? Running before the actual merge? I suppose in that case this check might start to complain.

@psss psss added the full test Pull request is ready for the full test execution label May 9, 2024
Copy link
Collaborator

@martinhoyer martinhoyer left a comment

Choose a reason for hiding this comment

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

Just a quick question - why not add it directly to the workflow env: key?

@psss psss changed the title Run the no-commit-to-branch check for pull requests only Run the no-commit-to-branch check only locally May 9, 2024
@psss psss requested a review from martinhoyer May 9, 2024 13:35
@psss
Copy link
Collaborator Author

psss commented May 9, 2024

Just a quick question - why not add it directly to the workflow env: key?

Yeah, now that the condition was dropped this can be simplified. Updated.

It does not make sense to run this `pre-commit` check in pull
requests or when merging to the `main` branch on GitHub. Also
update actions to the latest versions to get rid of warnings.
@psss
Copy link
Collaborator Author

psss commented May 9, 2024

Also updated to the latest versions of actions to get rid of the warnings.

@psss psss self-assigned this May 9, 2024
@psss psss merged commit 40ec859 into main May 9, 2024
12 of 21 checks passed
@psss psss deleted the no-commit-to-branch branch May 9, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full test Pull request is ready for the full test execution process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants