Skip to content

fix: Use dorny changes files instead of compromised tj-actions one#22359

Closed
andrii-korotkov-verkada wants to merge 4 commits intoargoproj:masterfrom
andrii-korotkov-verkada:use-dorny-changed-files
Closed

fix: Use dorny changes files instead of compromised tj-actions one#22359
andrii-korotkov-verkada wants to merge 4 commits intoargoproj:masterfrom
andrii-korotkov-verkada:use-dorny-changed-files

Conversation

@andrii-korotkov-verkada
Copy link
Copy Markdown
Contributor

@andrii-korotkov-verkada andrii-korotkov-verkada commented Mar 15, 2025

The later was compromised and taken down, the former seems like an almost drop-in replacement

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@andrii-korotkov-verkada andrii-korotkov-verkada requested review from a team as code owners March 15, 2025 15:39
@bunnyshell
Copy link
Copy Markdown

bunnyshell bot commented Mar 15, 2025

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@andrii-korotkov-verkada andrii-korotkov-verkada force-pushed the use-dorny-changed-files branch 3 times, most recently from 11f5a31 to 97ba8f5 Compare March 15, 2025 15:44
The later was compromised and taken down, the former seems like a drop-in replacement

Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
Address tests flakiness by adjusting batch event processing time to 1ms in e2e tests (as there's little to process) and waiting for 5ms in When and Then to account for that delay.

Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
@blakepettersson
Copy link
Copy Markdown
Member

Now I remember why I originally didn't go with dorny 😢 (see #17180)

dorny/paths-filter doesn't seem to handle (multiple) negations well. Therefore, this PR switches to tj-actions/changed-files, since it is already successfully used in argo-workflows.

@blakepettersson
Copy link
Copy Markdown
Member

See dorny/paths-filter#45, although IIRC AND-ing everything on one line didn't really work either.

@andrii-korotkov-verkada
Copy link
Copy Markdown
Contributor Author

Let me see if I can avoid negations

@andrii-korotkov-verkada
Copy link
Copy Markdown
Contributor Author

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@bfd72b4). Learn more about missing BASE report.
Report is 421 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master   #22359   +/-   ##
=========================================
  Coverage          ?   55.84%           
=========================================
  Files             ?      343           
  Lines             ?    57323           
  Branches          ?        0           
=========================================
  Hits              ?    32010           
  Misses            ?    22656           
  Partials          ?     2657           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Andrii Korotkov <andrii.korotkov@verkada.com>
@andrii-korotkov-verkada andrii-korotkov-verkada added the ready-for-review An approver should give a final review and merge the PR label Mar 15, 2025
@github-project-automation github-project-automation bot moved this to Ready for final review in Argo CD Review Mar 15, 2025
Copy link
Copy Markdown
Contributor

@todaywasawesome todaywasawesome left a comment

Choose a reason for hiding this comment

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

This project might be abandoned with no releases in the last year. dorny/paths-filter#262

@andrii-korotkov-verkada
Copy link
Copy Markdown
Contributor Author

@todaywasawesome, the last release was slightly more than a year ago, which is still not too bad for a simple action. We use some libs in Argo which are 5+ years old and not maintained, which is still often okay due to their simplicity.

@alexymantha
Copy link
Copy Markdown
Member

For reference, tj-actions/changed-files has been reinstated on Sunday (Mar 17) and there is an issue discussing it here: tj-actions/changed-files#2464. Here is the response from one of the maintainers: tj-actions/changed-files#2464 (comment)

In another comment, they also mentioned that whey will require signed commits

My 2 cents: these things happen, accounts get compromised, dependencies get attacked, repos get hijacked unfortunately pretty frequently. I think their response to this whole thing is more important than the fact that it happened. If they try to learn from it, take action and put measures in place to make sure this doesn't happen, then I think it's worth considering to keep using the action.

The fact that we are pinning the action to a specific commit also makes it fairly safe, as long as the action is vetted when the commit SHA is updated.

I also don't think that replacing it with a seemingly abandoned action just for the sake of not using an action that has been compromised in the past is the right move here. tj-actions/changed-files is an active project, it's higher profile and it has more eyes on it.

@andrii-korotkov-verkada
Copy link
Copy Markdown
Contributor Author

Thanks for your response. I'm still concerned that nobody is able to tell how the PAT got compromised. Yet I appreciate how they handled the response.

@andrii-korotkov-verkada
Copy link
Copy Markdown
Contributor Author

A consensus on a thread seems to be to keep using tj-actions action, so closing this https://cloud-native.slack.com/archives/C020XM04CUW/p1742051731832059

@github-project-automation github-project-automation bot moved this from Ready for final review to Done in Argo CD Review Mar 17, 2025
@andrii-korotkov-verkada andrii-korotkov-verkada deleted the use-dorny-changed-files branch March 17, 2025 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review An approver should give a final review and merge the PR

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants