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

Unexpected branch values are used for recordings produced by GitHub Actions #27389

Closed
tbiethman opened this issue Jul 25, 2023 · 1 comment · Fixed by #27409
Closed

Unexpected branch values are used for recordings produced by GitHub Actions #27389

tbiethman opened this issue Jul 25, 2023 · 1 comment · Fixed by #27409

Comments

@tbiethman
Copy link
Contributor

tbiethman commented Jul 25, 2023

Current behavior

For cloud recordings, the app uses data from the execution environment to supplement data it collects from git directly to decorate the recorded run. For the case of GitHub Actions, one of the environment variables we read to determine the current branch name (GITHUB_REF) provides unexpected values. See the following table, which contains environment data for a branch named gha-testing triggered for different event types:

GITHUB_EVENT_NAME GITHUB_REF GITHUB_REF_NAME GITHUB_HEAD_REF
push refs/heads/gha-testing gha-testing N/A (unset for push)
pull_request refs/pull/13/merge 13/merge gha-testing

Notice that the GITHUB_REF value, regardless of the triggering event, is always a refs/* value that doesn't accurately represent the branch as labeled (gha-testing).

Desired behavior

The app should utilize other variables available to it to record the true branch name, whenever possible.

To accomplish this, I propose the app should read the following environment variables, in order of precedence:

  1. GH_BRANCH - This is the current prioritized value. It is conditionally set by the Cypress github-action. When it is set, the value is correct, so I don't see a reason to remove/reorder its position in our logic for anyone else that may be utilizing it as an override.
  2. GITHUB_HEAD_REF - This env is set for pull_request-related workflow events. In these cases, it holds the desired HEAD branch value. It is otherwise unset.
  3. GITHUB_REF_NAME - This env is set for all branch-related workflows, but is most relevant for non-pull_request-related workflows. In these cases, the value is the true HEAD branch value. This value can also be used as the last check, as there is no other environment variable that could provide us with more accurate data at this time.

In summary, we should update the logic here to be:

branch: env.GH_BRANCH || env.GITHUB_HEAD_REF || env.GITHUB_REF_NAME

Test code to reproduce

See the environment output from my repository's workflow executions:

  1. pull_request event - https://github.com/tbiethman/cypress-test-tiny/actions/runs/5649844024/job/15305027983#step:8:16
  2. push event - https://github.com/tbiethman/cypress-test-tiny/actions/runs/5649843632/job/15305026829#step:8:16

Cypress Version

Current (all way back to v2.2.4)

Node version

v16.16.0

Operating System

Ubuntu 22.04.2 (in a GitHub Action workflow)

Debug Logs

See some sample output from the commit-info package at recording time for a pull_request triggered workflow. You'll notice the null branch value in the collected data, which causes the fallback logic described above to come into play.

commit-info git stdout: HEAD +11ms
  commit-info git stdout: Merge de00848379c8018c4f28ec0e0c0431241fd2aced into e25698dc3420d12e0eda205723eaec54cd02c439
 +1ms
  commit-info git stdout: tbiethman@users.noreply.github.com +0ms
  commit-info git stdout: Tyler Biethman +0ms
  commit-info git stdout: c[62](https://github.com/tbiethman/cypress-test-tiny/actions/runs/5649844024/job/15305028227#step:3:63)3683d165d8183fb27e71ed27bf4fd2f3938e9 +0ms
  commit-info git stdout: 1690232980 +0ms
  commit-info git stdout: https://github.com/tbiethman/cypress-test-tiny +0ms
  commit-info git commit: { branch: null, message: 'Merge de00848379c8018c4f28ec0e0c0431241fd2aced into e25698dc3420d12e0eda205723eaec54cd02c439\n', email: 'tbiethman@users.noreply.github.com', author: 'Tyler Biethman', sha: 'c623683d1[65](https://github.com/tbiethman/cypress-test-tiny/actions/runs/5649844024/job/15305028227#step:3:66)d8183fb27e71ed27bf4fd2f3938e9', timestamp: '1690232980', remote: 'https://github.com/tbiethman/cypress-test-tiny' } +65ms

Other

We have also had some discussion related to normalizing the environment's raw git data and environment variables within the recording service directly, essentially offloading the offending logic here into the cloud. This would give Cypress a better avenue to respond to changes in the various CI environments in the future without requiring a specific app release. A spike has been logged internally to investigate that option and any potential tradeoffs. The proposal described above seems like a quick enough win to get the correct data recorded in the meantime.

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Aug 1, 2023

Released in 12.17.3.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v12.17.3, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant