ci: Fix triggering CI

[manuzhang]

No description provided.

[kevinjqliu]

Thanks!

This is a follow up to #1686 (comment)

[kevinjqliu]

I think this fix the original issue. WDYT of using the catch-all with exception logic similar to

iceberg-rust/.github/workflows/ci.yml

Lines 24 to 40 in d66f994

	pull_request:
	branches:
	- '**' # Include all files and directories in the repository by default.
	- '!.github/workflows/**' # Exclude all workflow files
	- '.github/workflows/ci.yml' # except the current file.
	- '!.github/actions/**' # Exclude custom actions
	- '!.github/ISSUE_TEMPLATE/**' # Exclude files and directories that don't impact tests or code like templates, metadata, and documentation.
	- '!scripts/**'
	- '!website/**'
	- '!.asf.yml'
	- '!.gitattributes'
	- '!.gitignore'
	- '!CONTRIBUTING.md'
	- '!CHANGELOG.md'
	- '!LICENSE'
	- '!NOTICE'
	- '!README.md'

I see there are other files that might affect the python-ci, like the rust-toolchain.toml or rustfmt.toml

[manuzhang]

@kevinjqliu Sure, but I don't know why CI is triggered here. Do you have an idea?

[kevinjqliu]

ah! we're missing path here

iceberg-rust/.github/workflows/ci.yml

Lines 21 to 26 in d66f994

	push:
	branches:
	- main
	pull_request:
	branches:
	- '**' # Include all files and directories in the repository by default.

see this example from pyiceberg https://github.com/apache/iceberg-python/blob/83789f0f078005e347ac866bc8f9b9402eabd270/.github/workflows/python-ci.yml#L22-L29
and the docs here https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#onpushpull_requestpull_request_targetpathspaths-ignore

Thanks LLM for catching this.

[manuzhang]

@kevinjqliu Thanks, silly mistake. On the other hand, I don't want to trigger Bindings Python CI on all source code changes. If we add other modules in the future, we might need to modify this again.

[kevinjqliu]

I like the include all and then exclude file approach. think we can change .github/workflows/bindings_python_ci.yml to use that?
I think its better to run the CI checks more often for correctness. And in some cases, we can optimize and exclude files

I think ppl also wont realize that this logic is here. so its better to "catch all". WDYT?

[liurenjie1024]

Is this safe? I don't think we should exclude workflow files.

[manuzhang]

This combined with the following line includes only binding_python_ci.yml to trigger this workflow, while changes to other workflow files will not. Make sense?

[liurenjie1024]

I don't think it makes sense to exclude workflow changes, which may cause confusions. We should be careful when excluding files.

[liurenjie1024]

Same as above.

[manuzhang]

Can you explain how these actions are related to the bindings/python module?

[liurenjie1024]

Also why we don't run ci when we changes actions?

[liurenjie1024]

I don't think we should exclude this.

[manuzhang]

This combined with the following two lines include crates/iceberg/** and crates/integrations/datafusion/**, which are depended on by bindings/python, to trigger this workflow.

[liurenjie1024]

When we change codes in crates, it may also affect python binding, see bindings/python/Cargo.toml:35

[manuzhang]

Yes, crates/iceberg and crates/integrations/datafusion are included below.

[liurenjie1024]

What I want to say is that we should not only focus on current status. For example, what if we added another crate and python binding relies on it, and the ci may not be triggerred. Yes you could argue that this could be resolved eventually, but it would be confusing for new contributor to see that ci can't detect the changes.

[liurenjie1024]

Same as python ci, please restore workflows, actions.