Skip to content

Conversation

@Thedarkmatter10
Copy link
Contributor

@Thedarkmatter10 Thedarkmatter10 commented Jun 12, 2025

🔧 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_SDK was defined repeatedly in multiple steps.

After:

Declared in gloabally one time.

Why This Matters:

  • Eliminates duplication across steps.
  • Makes it easier to update or remove the variable in the future.
  • Still allows per-step override when necessary.

2. Fix Unsafe Shell Conditional on inputs.release

3. Removed EXCLUSION AND TODO comment.

Affected Steps:

check Formatting

make proto

Improvements:

Now that the repositories are public, we no longer need to exclude files like Package.swift and Package.resolved from formatting and proto checks.

  • Removed EXCLUDES logic
  • Removed related TODO comments
  • Updated git diff checks to include all files

@wlan0 @katiewasnothere

Comment on lines 47 to 51
Copy link
Contributor

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.

Copy link
Contributor Author

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!!

@katiewasnothere
Copy link
Contributor

Please run make fmt then it LGTM :)

@Thedarkmatter10
Copy link
Contributor Author

I’m on Windows and don’t have Swift installed, so I’m unable to run make fmt for the Package.swift file. Could you please help format it on your end, or let me know an alternative workaround?
@katiewasnothere

@katiewasnothere
Copy link
Contributor

@Thedarkmatter10 Can you rebase with main again? I think that should resolve the formatting error

@Thedarkmatter10 Thedarkmatter10 force-pushed the fix/common-yml-sdk-shell-check-cleanup branch from 876591d to 91920ad Compare June 13, 2025 19:52
@Thedarkmatter10
Copy link
Contributor Author

Rebased with main as suggested. Please let me know if anything else is needed!
@katiewasnothere

@katiewasnothere katiewasnothere merged commit 2473016 into apple:main Jun 13, 2025
2 checks passed
env:
DEVELOPER_DIR: "/Applications/Xcode_16.3.app/Contents/Developer"
CURRENT_SDK: y
CURRENT_SDK: y # explicitly repeated due to local env block
Copy link
Contributor

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?

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.

3 participants