-
Notifications
You must be signed in to change notification settings - Fork 584
fix(common.yml): globalize CURRENT_SDK, improve shell safety and imp… #178
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
fix(common.yml): globalize CURRENT_SDK, improve shell safety and imp… #178
Conversation
.github/workflows/common.yml
Outdated
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.
Hi thanks for the fix! We can actually just remove these exclusions in this step and the formatting step. This was needed while we were testing because the repos were private. Now that they're public, we should not have these exclusions.
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.
Hi, I've removed the temporary exclusions from both the formatting and protobuf check steps, as suggested. The workflow now checks all files without skipping any, since the repositories are public. I also cleaned up the related TODO comments .
Let me know if any further adjustments are needed. Thanks!!
|
Please run |
|
I’m on Windows and don’t have Swift installed, so I’m unable to run |
|
@Thedarkmatter10 Can you rebase with main again? I think that should resolve the formatting error |
876591d to
91920ad
Compare
|
Rebased with main as suggested. Please let me know if anything else is needed! |
| env: | ||
| DEVELOPER_DIR: "/Applications/Xcode_16.3.app/Contents/Developer" | ||
| CURRENT_SDK: y | ||
| CURRENT_SDK: y # explicitly repeated due to local env block |
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.
@katiewasnothere Do we still need CURRENT_SDK?
🔧 Improvements Summary
This PR introduces three improvements focused on safety, maintainability, and readability of the GitHub Actions workflow.
1. Define Global Environment Variable
Before:
Environment variable
CURRENT_SDKwas defined repeatedly in multiple steps.After:
Declared in gloabally one time.
Why This Matters:
2. Fix Unsafe Shell Conditional on inputs.release
PREVIOUSLY FIXED Containerization project. https://github.com/apple/containerization/pull/68
3. Removed EXCLUSION AND TODO comment.
Affected Steps:
check Formattingmake protoImprovements:
Now that the repositories are public, we no longer need to exclude files like Package.swift and Package.resolved from formatting and proto checks.
@wlan0 @katiewasnothere