-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Run config_docs CI check on PRs to change auto generated docs #17046
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
Conversation
| - "docs/source/user-guide/configs.md" | ||
| - "docs/source/user-guide/sql/aggregate_functions.md" | ||
| - "docs/source/user-guide/sql/scalar_functions.md" | ||
| - "docs/source/user-guide/sql/window_functions.md" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easy to miss adding a new document here.
Can we run this workflow on any changes to the user guide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are the only files that are auto generated (and thus the only .md files that can be affected by the change in the rust code). I was trying to preserve the property that most changes to the docs did not require all the rust tests to be run
| pull_request: | ||
| paths: | ||
| - ".github/**" | ||
| - "datafusion/**" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cargo.toml, cargo.lock, rust-toolchain.toml, ?
| container: | ||
| image: amd64/rust | ||
| steps: | ||
| - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think actions/checkout is controlled by GitHub. I believe it's safe to use tag reference on this one and the setup-node as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is copyied from the existing jobs
They were pinned to shas in this PR by @gopidesupavan i think as an added level of protection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@findepi yes agree thought its managed from github tags will not protect us from any attacks if something wrong or anyone manipulates the pinned tag, commit sha is preffered way :) and its recommended way from ASF policy aswell https://infra.apache.org/github-actions-policy.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i am well aware of 3rd party github actions being an "awesome" (easy) attack vector when they are not pinned to a sha. However, i think GH recommends this for 3rd party actions, not github-owned actions
from ASF page linked above
You MAY use all actions internal to the apache/, github/ and actions/* namespaces without restrictions.
You MUST pin all external actions to the specific git hash (SHA1) of
I interpret it as saying "actions./*" is "internal" and you don't need to pin to a sha.
While we MAY pin also internal actions, it can be viewed as unnecessary hassle when updating to a newer version.
Co-authored-by: Piotr Findeisen <[email protected]>
|
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days. |
Which issue does this PR close?
Rationale for this change
We now broke CI twice in the last day with docs changes that did not run the config_docs check.
Here are two PRs that fixed it:
(Thanks to @liamzwbao and @adamreeve for pointing them out)
let's fix the docs check to automatically run on PRs that change the auto generated files
What changes are included in this PR?
Are these changes tested?
I am testing them manually
Are there any user-facing changes?
No, this is a CI process change only