Skip to content
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

Replace change detection GitHub Action #12188

Merged
merged 1 commit into from
Mar 15, 2025
Merged

Replace change detection GitHub Action #12188

merged 1 commit into from
Mar 15, 2025

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Mar 15, 2025

Summary

tj-actions/changed-files no longer exists due to a malicious commit. This PR replaces it with a minimal shell script to get us unblocked.

@charliermarsh charliermarsh force-pushed the charlie/change branch 3 times, most recently from db0fc27 to dd7e0b0 Compare March 15, 2025 16:00
@charliermarsh charliermarsh requested a review from zanieb March 15, 2025 16:05
@charliermarsh charliermarsh marked this pull request as ready for review March 15, 2025 16:05
@charliermarsh charliermarsh added testing Internal testing of behavior internal A refactor or improvement that is not user-facing and removed testing Internal testing of behavior labels Mar 15, 2025
@charliermarsh charliermarsh changed the title Replace change detection Replace change detection GitHub Action Mar 15, 2025
while IFS= read -r file; do
# Skip if file matches exclusion patterns.
if [[ "$file" =~ ^docs/ && ! "$file" =~ ^docs/reference/(cli|settings).md && ! "$file" =~ ^docs/configuration/environment.md ]]; then
echo "Skipping $file (matches `docs/` pattern)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These backticks are going to evaluate

❯ echo "foo `bar`"
zsh: command not found: bar
foo 

Copy link
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


while IFS= read -r file; do
# Skip if file matches exclusion patterns.
if [[ "$file" =~ ^docs/ && ! "$file" =~ ^docs/reference/(cli|settings).md && ! "$file" =~ ^docs/configuration/environment.md ]]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use "${file}" throughout — I don't think there's an attack vector here because of the way you're iterating but it's safer.

CODE_CHANGED=true
break

done <<< "$CHANGED_FILES"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
done <<< "$CHANGED_FILES"
done <<< "${CHANGED_FILES}"

break

done <<< "$CHANGED_FILES"
echo "code_any_changed=$CODE_CHANGED" >> "$GITHUB_OUTPUT"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
echo "code_any_changed=$CODE_CHANGED" >> "$GITHUB_OUTPUT"
echo "code_any_changed=${CODE_CHANGED}" >> "$GITHUB_OUTPUT"

- "!**/*.md"
- "!bin/**"
- "!assets/**"
# Generated markdown and JSON files are checked during test runs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth retaining the comment on why we run tests on these files

@charliermarsh charliermarsh merged commit dab1ea2 into main Mar 15, 2025
76 checks passed
@charliermarsh charliermarsh deleted the charlie/change branch March 15, 2025 17:12
@vladyslav-burylov
Copy link

Hi, @charliermarsh. First of all, thank you for the above remediating security risk quickly.

Could you please kindly comment if you have plans to disclose some sort of post-mortem outlining potential impact on uv users? Are there any actions which should be done on user side to remediate impact of potentially leaked uv CI secrets?

@charliermarsh
Copy link
Member Author

Good question: There's no action required on the user side and no impact to our users. We rotated all secrets in the repository, and no secrets that facilitate PyPI publishing were exposed since we use Trusted Publishing.

@vladyslav-burylov
Copy link

Good! Thank you so much for quick response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal A refactor or improvement that is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants