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

Set up pre-commit hooks and added configuration files #1

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

Al-an-21
Copy link
Contributor

@Al-an-21 Al-an-21 commented Nov 8, 2024

PR introduces pre-commit hooks and updates the layer.conf file to ensure compatibility with the specified Yocto series.

@Al-an-21 Al-an-21 requested a review from macpijan November 8, 2024 13:14
@Al-an-21 Al-an-21 self-assigned this Nov 8, 2024
@macpijan macpijan requested a review from PLangowski November 8, 2024 13:18
autoupdate_commit_msg: 'pre-commit: autoupdate hooks'
autofix_prs: false
# docker is not supported on pre-commit.ci
skip: [shellcheck]
Copy link
Member

Choose a reason for hiding this comment

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

@Al-an-21 CI breaks on this step, let's check whether it's needed here/on this repo

Copy link
Contributor

Choose a reason for hiding this comment

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

This skip makes sense only if we use shellcheck later in the hooks. Here we do not use it, so it complains it does not know which hook to skip, since it is not defined.

@Al-an-21
Copy link
Contributor Author

Al-an-21 commented Nov 8, 2024

According to this comment from Paweł, I was under the impression that the setup was correct. However, I’ve noticed that CI is not currently configured for this repository. Clarification on which specific step might be causing issues, as well as any particular requirements for the CI process, would be really helpful.

Additionally, I’ve observed similar failures in other PRs, like here, where it also fails.

At this point, I’m feeling a bit unsure about the situation and what the appropriate next steps should be. Any guidance or insight would be greatly appreciated.

@macpijan
Copy link
Contributor

However, I’ve noticed that CI is not currently configured for this repository.

What makes you think so?

Clarification on which specific step might be causing issues, as well as any particular requirements for the CI process, would be really helpful.

Error logs from CI are really helpful in this matter: https://results.pre-commit.ci/run/github/884699586/1731079763.yDE45quGRTSrFpXri4k-0g

==> File .pre-commit-config.yaml
==> At Config()
==> At key: ci
==> At key: skip
=====> unexpected hook ids: shellcheck

So you should simply remove this: https://github.com/zarhus/meta-coreboot/pull/1/files#r1834676377

It's been suggested by @artur-rs 4 days ago as a solution to your problem already.

Sounds to me like shellcheck is used in skip, but is never defined.

@PLangowski
Copy link

I didn't notice the mistake in the config. I wonder why there were no errors when I ran the pre-commit locally.
@macpijan Do we not expect any shell scripts in this repo?

@macpijan
Copy link
Contributor

I wonder why there were no errors when I ran the pre-commit locally.

Because it is in the ci section, which is relevant for pre-commit.ci only.

Do we not expect any shell scripts in this repo?

I do not think so. Definitely not at the moment.


default_install_hook_types: [pre-commit, commit-msg]

#ci:

Choose a reason for hiding this comment

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

Maybe you shouldn't delete the entire section? Just skip: [shellcheck]. Also, you should delete it instead of commenting it out

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@macpijan macpijan merged commit 0cac162 into main Nov 14, 2024
1 check passed
@macpijan macpijan deleted the pre-commit-setup branch November 14, 2024 13:56
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