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

Scripts as precommit hook #1473

Merged
merged 6 commits into from
Nov 27, 2023
Merged

Scripts as precommit hook #1473

merged 6 commits into from
Nov 27, 2023

Conversation

MarcelKoch
Copy link
Member

This PR make the pre-commit hook call the scripts format_header.sh and ginkgo_update_header.sh. It was necessary to modify format_header.sh to accept multiple files.

Additionally, the .pre-commit-config.yaml is now correctly staged before pre-commit is run.

@MarcelKoch MarcelKoch added reg:ci-cd This is related to the continuous integration system. 1:ST:ready-for-review This PR is ready for review 1:ST:skip-full-test labels Nov 27, 2023
@MarcelKoch MarcelKoch self-assigned this Nov 27, 2023
@ginkgo-bot ginkgo-bot added the reg:helper-scripts This issue/PR is related to the helper scripts mainly concerned with development of Ginkgo. label Nov 27, 2023
@upsj
Copy link
Member

upsj commented Nov 27, 2023

This means #1466 isn't sufficient and should be closed? I'd be really happy if we could make that work without manual scripts

@MarcelKoch
Copy link
Member Author

@upsj #1466 will be a successor to this. I just wanted to make the CI work again, with minimal changes. I'm still planning to remove the format_header.sh script.

@upsj
Copy link
Member

upsj commented Nov 27, 2023

nit: you should change the API token back to the @ginkgo-bot token we have stored as a secret

@upsj
Copy link
Member

upsj commented Nov 27, 2023

I think you could also use this to remove rebase! and format-rebase!

Copy link

Error: The following files need to be formatted:

.clang-format
.pre-commit-config.yaml

You can find a formatting patch under Artifacts here or run format! if you have write access to Ginkgo

Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

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

LGTM! Can you confirm the only changes you did to format_header.sh are the outer for-loop and replacing $1 by ${current_file}?

@ginkgo-project ginkgo-project deleted a comment from github-actions bot Nov 27, 2023
@MarcelKoch
Copy link
Member Author

LGTM! Can you confirm the only changes you did to format_header.sh are the outer for-loop and replacing $1 by ${current_file}?

I tested it locally with multiple inputs and it worked as expected. But there is no way to show this, except putting this in my fork.

@MarcelKoch MarcelKoch requested a review from a team November 27, 2023 12:20
@upsj
Copy link
Member

upsj commented Nov 27, 2023

@MarcelKoch I just meant that the diff makes it a bit unclear whether other things changed except for the indentation. But I see that that's not the case

@MarcelKoch MarcelKoch merged commit 6554f69 into develop Nov 27, 2023
9 of 15 checks passed
@MarcelKoch MarcelKoch deleted the scripts-as-precommit-hook branch November 27, 2023 14:04
Copy link

sonarcloud bot commented Nov 27, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

warning The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

@upsj upsj added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. 1:ST:skip-full-test reg:ci-cd This is related to the continuous integration system. reg:helper-scripts This issue/PR is related to the helper scripts mainly concerned with development of Ginkgo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants