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

Fixing semver checks again #384

Merged
merged 10 commits into from
Oct 9, 2024

Conversation

nicklan
Copy link
Collaborator

@nicklan nicklan commented Oct 9, 2024

The saga continues.

Turns out pull_request_target triggers mean that the checkout action will checkout the head of main, not the head of the PR, so the check was a no-op because it was comparing main to main and nothing changed.

So, now we specify a ref in the checkout action and point it at github.event.pull_request.head.sha which afaict will checkout the head of the PR.

That means the running the job with write permissions would be dangerous because any code in the PR could be evaluated, so I've also split this into two jobs. One that does the check and needs the PR code, which runs with only read permissions, and then a second job that looks at the output of the previous job and just adds a label or not and nothing else, so it can have write permissions.

Tested as best I can via using a "normal" pull_request trigger. You can see the expected run here where it got as far as correctly wanting to add the label, but failing due to permissions (which will be fixed by merging this)

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.66%. Comparing base (42afc06) to head (6b84489).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #384   +/-   ##
=======================================
  Coverage   77.66%   77.66%           
=======================================
  Files          49       49           
  Lines       10079    10079           
  Branches    10079    10079           
=======================================
  Hits         7828     7828           
  Misses       1805     1805           
  Partials      446      446           

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

@nicklan nicklan force-pushed the test-semver-check-wprtarget branch from bf9d962 to 60863fe Compare October 9, 2024 22:31
@nicklan nicklan changed the title testing semver checks again Fixing semver checks again Oct 9, 2024
@nicklan nicklan marked this pull request as ready for review October 9, 2024 22:36
Copy link
Collaborator

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

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

nice catch on the security concern!

Copy link
Collaborator

@OussamaSaoudi-db OussamaSaoudi-db left a comment

Choose a reason for hiding this comment

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

Moving PR: write to a just the labeler is nice. GJ

@nicklan nicklan force-pushed the test-semver-check-wprtarget branch from 60863fe to 6b84489 Compare October 9, 2024 23:27
@nicklan nicklan merged commit f485466 into delta-incubator:main Oct 9, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants