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

chore: introduce black for code format in github actions #47

Merged
merged 8 commits into from
Jun 6, 2024

Conversation

ChenZiHong-Gavin
Copy link
Contributor

According to #46, I think black for code format can be supported by black integration with github actions(black.readthedocs.io/en/stable/integrations/github_actions.html).

the workflow fails if Black finds files that need to be formatted, as can be seen in ChenZiHong-Gavin#17
image

so i think everyone should run black in ./style/code_format_and_analysis.sh before commit. (Perhaps it would be better to automatically trigger [the process] through GitHub Hooks rather than running it manually.)

after that, Black passes the workflow, as can be seen in ChenZiHong-Gavin#18
image

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. enhancement New feature or request labels May 30, 2024
.github/workflows/black.yml Outdated Show resolved Hide resolved
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: psf/black@stable
Copy link
Member

@imbajin imbajin Jun 3, 2024

Choose a reason for hiding this comment

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

Come to think of it, ASF/Apache seems to have some rules about external action, and those don't meet the requirements may be banned 🚫 (I found ↓)

image

https://infra.apache.org/github-actions-policy.html

(maybe we could search some apache's python project that use the same action/step?)


Perhaps this is also the reason why we didn't see Black in our PR CI? (But it works fine in ur personal repo ↓)

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I'll check that later~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image
that is why Black action is not showing

Copy link
Member

@imbajin imbajin Jun 4, 2024

Choose a reason for hiding this comment

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

image that is why Black action is not showing

@liuxiaocs7 @simon824 any suggestion?

(I forgot in which ASF project I saw a way to indirectly reference an action, partially bypassing this restriction)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we have 2 approaches:

  1. follow the rules and use the specific git hash (SHA1) of the action, for example psf/black@3702ba224ecffbcec30af640c149f231d90aebdb
  2. use pip install & bash as https://github.com/apache/incubator-hugegraph-ai/blob/main/.github/workflows/pylint.yml did

I prefer approach 1.

Also, i found a ASF repo which may not obey the restriction but still confused how they did it.
In https://github.com/apache/shenyu/blob/master/.github/workflows/e2e-k8s.yml
image

Copy link
Member

Choose a reason for hiding this comment

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

I think we have 2 approaches:

  1. follow the rules and use the specific git hash (SHA1) of the action, for example psf/black@3702ba224ecffbcec30af640c149f231d90aebdb
  2. use pip install & bash as main/.github/workflows/pylint.yml did

I prefer approach 1.

Also, i found a ASF repo which may not obey the restriction but still confused how they did it. In apache/shenyu@master/.github/workflows/e2e-k8s.yml image

Thanks for ur feedback~

U could try way1 first (This seems to be a new rule added this year, at least I didn't see it last year)

@imbajin
Copy link
Member

imbajin commented Jun 5, 2024

😿
image

@ChenZiHong-Gavin
Copy link
Contributor Author

😿 image

Strange here, i think psf/black@3702ba224ecffbcec30af640c149f231d90aebdb already matched */*@[a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9][a-f0-9]+

Copy link
Member

@imbajin imbajin left a comment

Choose a reason for hiding this comment

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

Merge it first & test it?

(Also I noticed GitHub Action doesn't support short hash value like 3702ba2 for now :)

Update:
Also failed in the latest CI/code (maybe we could submit a ticket in ASF-JIRA for the reason, you need register it first)
image

@imbajin imbajin merged commit 020f99b into apache:main Jun 6, 2024
11 checks passed
Copy link
Member

@imbajin imbajin Jun 6, 2024

Choose a reason for hiding this comment

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

Note: next time we should enhance the filename black.yml (which is not clear enough)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok


on:
push:
branches:
Copy link
Member

Choose a reason for hiding this comment

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

and shall we add main branch here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants