Skip to content

Conversation

jescalada
Copy link
Contributor

This is a solution to some problems identified by @kriswest in #973 after merging in the latest security fixes.

I originally wrote getMissingData to solve the problem when making commits from an unapproved branch, in which case action.commitData and action.user are empty and must be fetched later. This means the user checks are deferred, and some checks were inadventently skipped as well.

To sum it up, getMissingData was unsafe, due to mistakenly setting the committer to the author_name which is not always true, and it wasn't actually necessary since Fabio's checkHiddenCommits action can handle the edge case mentioned above. I decided to keep only the empty branch check which is still useful and hopefully safe.

Changelog

  • Remove getMissingData action
  • Add checkEmptyBranch action to handle missing commitData and empty branch identification
    • I decided to do this separately since parsePush is quite bloated already
  • Add parsePush test for demonstrating that commitData == [] on empty branch pushes
    • There may be other cases where this is true, such as --delete commands, but these are not supported yet

Copy link

netlify bot commented Aug 3, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 0d5b5aa
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/6892c3b93205460009250d8c

Copy link

codecov bot commented Aug 3, 2025

Codecov Report

❌ Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.28%. Comparing base (43c6497) to head (0d5b5aa).
⚠️ Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
...c/proxy/processors/push-action/checkEmptyBranch.ts 96.15% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1134      +/-   ##
==========================================
+ Coverage   79.62%   81.28%   +1.65%     
==========================================
  Files          59       59              
  Lines        2479     2458      -21     
  Branches      289      279      -10     
==========================================
+ Hits         1974     1998      +24     
+ Misses        461      416      -45     
  Partials       44       44              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jescalada jescalada self-assigned this Aug 4, 2025
@jescalada jescalada requested a review from kriswest August 4, 2025 12:41
@jescalada
Copy link
Contributor Author

@kriswest Would love to get a quick look on this PR - it should deal with the issue that you mentioned in #973. If this clears things up then we should be able to merge your PR for 2.0.0-rc.1!

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

This looks good to me - thank you for taking it on so quickly. One very minor comment and a question.

@jescalada jescalada requested a review from kriswest August 6, 2025 02:30
@jescalada jescalada merged commit 9913250 into finos:main Aug 7, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants