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

Update Base SHA for Dangerfile checks #20032

Merged
merged 6 commits into from
Dec 31, 2024

Conversation

ryan-mcneil
Copy link
Contributor

@ryan-mcneil ryan-mcneil commented Dec 26, 2024

Summary

  • Before now, DANGER checks would always run against origin/master, so in the cases where a feature branch was the base branch, one would get unexpected results. For example if someone created a PR into a feature branch that changed 100 LOC, but the base branch already had 500+ changes, it would display a LOC error, despite it being in the acceptable range. This PR detects the BASE_SHA from the PR CI ENV.
  • Note that these vars only exist in GITHUB ACTION runs, but perhaps not in other contexts, so I default to the original ways we were pulling the base/target, just in case
  • I also removed the danger options --head=${{ github.sha }} --base=${{ github.event.pull_request.base.sha }} because they aren't actually being used (from what I can tell). It seems they were intending to solve the problem this PR is fixing, but never worked properly.

Testing done

  • Here is a PR(feature/base branch) with an LOC change that's firing off warnings. From that I created a second branch/PR, only changing these ENV vars, and note that it is not detecting the large LOC change as it's looking at the change between it and the base branch/PR, not master.
  • NOTE: Originally there was an issue and it was failing silently. Those issues are no longer present
  • Here's another example branched off of this PR's branch. Note the single commit with the high LOC change and the action properly notifying

@va-vfs-bot va-vfs-bot temporarily deployed to rm-update-dangerfile-base-branch/main/main December 26, 2024 18:24 Inactive
@ryan-mcneil ryan-mcneil changed the title WIP - dangerfile update Update Base SHA for Dangerfile checks Dec 26, 2024
@ryan-mcneil ryan-mcneil marked this pull request as ready for review December 26, 2024 18:51
@ryan-mcneil ryan-mcneil requested a review from a team as a code owner December 26, 2024 18:51
rmtolmach
rmtolmach previously approved these changes Dec 30, 2024
Copy link
Contributor

@rmtolmach rmtolmach left a comment

Choose a reason for hiding this comment

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

reviewed in CoP meeting ✅

@va-vfs-bot va-vfs-bot temporarily deployed to rm-update-dangerfile-base-branch/main/main December 30, 2024 22:39 Inactive
Dangerfile Outdated
@@ -3,8 +3,8 @@
require 'ostruct'

module VSPDanger
HEAD_SHA = `git rev-parse --abbrev-ref HEAD`.chomp.freeze
BASE_SHA = 'origin/master'
HEAD_SHA = ENV.fetch('GITHUB_HEAD_REF') ? "origin/#{ENV.fetch('GITHUB_HEAD_REF')}" : `git rev-parse --abbrev-ref HEAD`.chomp.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have to check ENV.fetch('GITHUB_HEAD_REF') for empty/nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@LindseySaari I did that because if those vars ARE nil, it'll evaluate to "origin/", but I'd rather it default to 'git rev-parse --abbrev-ref HEAD'.chomp.freeze/origin/master in the case it is being ran outside of the GitHUB CI context (I believe it can be ran locally, per our convo yesterday).

rmtolmach
rmtolmach previously approved these changes Dec 31, 2024
@ryan-mcneil ryan-mcneil merged commit 9257976 into master Dec 31, 2024
21 checks passed
@ryan-mcneil ryan-mcneil deleted the rm-update-dangerfile-base-branch branch December 31, 2024 18:08
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.

4 participants