Skip to content

Add pre-commit config#16

Merged
Fokko merged 6 commits into
apache:mainfrom
zhjwpku:add_pre-commit_config
Dec 23, 2024
Merged

Add pre-commit config#16
Fokko merged 6 commits into
apache:mainfrom
zhjwpku:add_pre-commit_config

Conversation

@zhjwpku

@zhjwpku zhjwpku commented Dec 17, 2024

Copy link
Copy Markdown
Collaborator

No description provided.

@zhjwpku zhjwpku force-pushed the add_pre-commit_config branch 3 times, most recently from fccea0e to 6adc9df Compare December 17, 2024 11:17
per request from apache#15

Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
@zhjwpku zhjwpku force-pushed the add_pre-commit_config branch from 6adc9df to 046430a Compare December 17, 2024 11:19

@Fokko Fokko left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Love it!

@raulcd raulcd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just a minor nit but LGTM. Thanks for the work @zhjwpku

Comment thread .github/workflows/pre-commit.yml Outdated
Co-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
@zhjwpku

zhjwpku commented Dec 17, 2024

Copy link
Copy Markdown
Collaborator Author

@Fokko @raulcd there is startup error, do you have any experience on this?
https://github.com/apache/iceberg-cpp/actions/runs/12374293318

Comment thread .github/workflows/pre-commit.yml
@raulcd

raulcd commented Dec 17, 2024

Copy link
Copy Markdown
Member

@Fokko @raulcd there is startup error, do you have any experience on this? https://github.com/apache/iceberg-cpp/actions/runs/12374293318

Oh, it seems pre-commit is not an allowed GH action to use, yes the snippet you shared looks good to me. That's more or less how we do it in Arrow: https://github.com/apache/arrow/blob/main/.github/workflows/dev.yml#L56-L67

edit: On another note we could ask infra to whitelist the action but that might take some time.

@zhjwpku

zhjwpku commented Dec 17, 2024

Copy link
Copy Markdown
Collaborator Author

@Fokko @raulcd there is startup error, do you have any experience on this? https://github.com/apache/iceberg-cpp/actions/runs/12374293318

Oh, it seems pre-commit is not an allowed GH action to use, yes the snippet you shared looks good to me. That's more or less how we do it in Arrow: https://github.com/apache/arrow/blob/main/.github/workflows/dev.yml#L56-L67

Yeah, I just copied that from Arrow with some reduction.

edit: On another note we could ask infra to whitelist the action but that might take some time.

Do you mean apache infra or github infra? I think we can go the Arrow way first and change to pre-commit action after infra whitelist it.

Signed-off-by: Junwang Zhao <zhjwpku@gmail.com>
@raulcd

raulcd commented Dec 17, 2024

Copy link
Copy Markdown
Member

you mean apache infra or github infra?

Apache Infra

@Fokko

Fokko commented Dec 17, 2024

Copy link
Copy Markdown
Contributor

Cool, I've raised an issue: https://issues.apache.org/jira/browse/INFRA-26378

@Fokko

Fokko commented Dec 17, 2024

Copy link
Copy Markdown
Contributor

Should we also add some instructions on how to use pre-commit? Similar to https://py.iceberg.apache.org/contributing/#linting

@Fokko

Fokko commented Dec 17, 2024

Copy link
Copy Markdown
Contributor

Cool, I've raised an issue: https://issues.apache.org/jira/browse/INFRA-26378

It got approved :)

@zhjwpku

zhjwpku commented Dec 18, 2024

Copy link
Copy Markdown
Collaborator Author

Cool, I've raised an issue: https://issues.apache.org/jira/browse/INFRA-26378

It got approved :)

Still got a startup error: https://github.com/apache/iceberg-cpp/actions/runs/12384969557, maybe we should just wait for some time ;(

@zhjwpku

zhjwpku commented Dec 18, 2024

Copy link
Copy Markdown
Collaborator Author

Should we also add some instructions on how to use pre-commit? Similar to https://py.iceberg.apache.org/contributing/#linting

I've added some linting instructions to Contribute section of README.md, please check if that is adequate.

@zhjwpku

zhjwpku commented Dec 23, 2024

Copy link
Copy Markdown
Collaborator Author

Cool, I've raised an issue: https://issues.apache.org/jira/browse/INFRA-26378

It got approved :)

Hi @Fokko, I see the comment that the pre-commit/action@3.0.1 has been whitelisted, do i need to take any further actions?

@Fokko

Fokko commented Dec 23, 2024

Copy link
Copy Markdown
Contributor

@zhjwpku no action needed, let me get this in

@Fokko Fokko merged commit 158d509 into apache:main Dec 23, 2024
@zhjwpku zhjwpku deleted the add_pre-commit_config branch December 23, 2024 12:56
@wgtmac

wgtmac commented Dec 27, 2024

Copy link
Copy Markdown
Member

pre-commit/action@v3.0.1 is not allowed to be used in apache/iceberg-cpp.

It seems that pre-commit check never run successfully: https://github.com/apache/iceberg-cpp/actions/workflows/pre-commit.yml. I saw the same thing with cpp-linter-action: https://issues.apache.org/jira/browse/INFRA-26318

@zhjwpku

zhjwpku commented Dec 27, 2024

Copy link
Copy Markdown
Collaborator Author

pre-commit/action@v3.0.1 is not allowed to be used in apache/iceberg-cpp.

It seems that pre-commit check never run successfully: https://github.com/apache/iceberg-cpp/actions/workflows/pre-commit.yml. I saw the same thing with cpp-linter-action: https://issues.apache.org/jira/browse/INFRA-26318

Yeah, I see that too, not sure how the infra works, any chance @Fokko take a look at this?

@lidavidm

Copy link
Copy Markdown
Member

Apache allow-lists GitHub actions to use. I think for arrow-adbc I've just manually installed and run pre-commit instead of using the dedicated action.

@zhjwpku

zhjwpku commented Dec 27, 2024

Copy link
Copy Markdown
Collaborator Author

Apache allow-lists GitHub actions to use. I think for arrow-adbc I've just manually installed and run pre-commit instead of using the dedicated action.

But https://issues.apache.org/jira/browse/INFRA-26378 seems whitelists the pre-commit action. Anyway, if we find that doesn't work in the end, I will file a PR changing to the arrow way.

@wgtmac

wgtmac commented Dec 27, 2024

Copy link
Copy Markdown
Member

I have replied to the JIRA ticket. I guess it might have some misconfigurations.

@Fokko

Fokko commented Dec 27, 2024

Copy link
Copy Markdown
Contributor

Looks like an issue on the INFRA end? For PyIceberg we also just install pre-commit and this works just as well

@wgtmac

wgtmac commented Dec 27, 2024

Copy link
Copy Markdown
Member

Yes, they have confirmed that there was a typo. Now it works: https://github.com/apache/iceberg-cpp/actions/runs/12512760344

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.

5 participants