-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix(ci): Check format for workflow yml files #26473
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
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsure the C++ format and header check workflow triggers on any GitHub workflow YAML change by broadening the path filter and correct minor formatting anomalies in the JDBC connector tests workflow file. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
494fb46 to
7c47231
Compare
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.
Hey there - I've reviewed your changes - here's some feedback:
- Consider scoping the prestocpp-format-and-header-check trigger more narrowly than
.github/workflows/**to avoid running the C++ format check on unrelated workflow changes. - Restore the missing newline at the end of
jdbc-connector-tests.ymlto maintain consistent file formatting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider scoping the prestocpp-format-and-header-check trigger more narrowly than `.github/workflows/**` to avoid running the C++ format check on unrelated workflow changes.
- Restore the missing newline at the end of `jdbc-connector-tests.yml` to maintain consistent file formatting.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The format checker is not called when yml files in the github workflow are modified but none in presto-native-execution. Thus, skipping changes that are found to be incorrect when the format check is called on the next PR.
7c47231 to
80e99a4
Compare
unidevel
left a comment
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.
LGTM
hantangwangd
left a comment
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.
Thanks @czentgr
An accidental change was made that change one of the value to persist credentials from false to true. This PR reverts this accidental change. Introduced by: #26473 ## Description <!---Describe your changes in detail--> ## Motivation and Context <!---Why is this change required? What problem does it solve?--> <!---If it fixes an open issue, please link to the issue here.--> ## Impact <!---Describe any public API or user-facing feature change or any performance impact--> ## Test Plan <!---Please fill in how you tested your change--> ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. - [ ] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == NO RELEASE NOTE == ```
The format checker is not called when yml files in the github workflow are modified but none in
presto-native-execution. Thus, skipping changes that are found to be incorrect when the format check is called on the next PR.
Description
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.