Fix continuous integration get base branch #32690
Conversation
This reverts commit d9b9acf.
…n-change-detection Fix Continuous Integration change detection
…ith Drone environment variable.
| @@ -93,11 +93,7 @@ def self.pr_base_branch_or_default_no_origin | |||
| end | |||
|
|
|||
| def self.circle_pr_branch_base_no_origin | |||
There was a problem hiding this comment.
It looks like this function is used in other places (such as eyes_steps.rb), so I'm not sure if it's safe to change this way. I'd be ok with just limiting the change to branch_to_base, though if you want to investigate to understand the situation / clean it up a bit, that'd be great also.
There was a problem hiding this comment.
Sorry I didn't take time to write up my analysis on the impact to other uses of this method. Here's what I found:
This method stopped working (it always returns nil now) when we switched from CircleCI to Drone because while Drone supports many CircleCI environment variables for cross-compatibility, it does not support the CI_PULL_REQUEST environment variable that CirceCI deprecated in Circle 2.0. This proposed change will result in the method working again as it did before the switch to Drone.
The Eyes steps no-op in Continuous Integration (Circle/Drone) builds, so the call to this method in the "I open my eyes" step always returns nil both before and after this proposed change.
I'm confident that the Eyes step does not need to be updated to account for this change.
There was a problem hiding this comment.
The Eyes logic that uses this method is a legacy attempt to implement branching logic for Eyes Baseline images, which Eyes now officially supports in its gem. When we revive and successfully implement feature branches for Eyes baseline images, the section of Eyes steps that invoke this method will be cleaned up.
There was a problem hiding this comment.
Can you do a little cleanup based on your findings, then? It sounds like we can just delete the call to it and the branch of the if statement which uses it from eyes_steps, is that right?
There was a problem hiding this comment.
Yes! Thanks for keeping me honest. This is a good opportunity to pay off some technical debt.
Description
Logic in Continuous Integration builds to get the base (target) branch of the Pull Request that triggered the build depended on a deprecated Circle environment variable.
Reviewer Checklist: