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

ci: Fix wrong ref in PR workflows which broke external builds #7431

Merged
merged 2 commits into from
Oct 13, 2023
Merged

ci: Fix wrong ref in PR workflows which broke external builds #7431

merged 2 commits into from
Oct 13, 2023

Conversation

MiloradFilipovic
Copy link
Contributor

@MiloradFilipovic MiloradFilipovic commented Oct 13, 2023

Github issue / Community forum post (link here to close automatically): #7423

This PR updates reference passed to the checkout action by the cy-pull-request.ym. This should fix three existing issues:

  • Failing unit tests for external pull requests
  • Failing e2e tests for external PRs
  • Passing empty ref to lint job which makes linter run on a wrong branch

@MiloradFilipovic MiloradFilipovic self-assigned this Oct 13, 2023
@n8n-assistant n8n-assistant bot added the n8n team Authored by the n8n team label Oct 13, 2023
Copy link
Member

@netroy netroy left a comment

Choose a reason for hiding this comment

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

Can you please update this in .github/workflows/e2e-tests-pr.yml as well 🙏🏽

@codecov
Copy link

codecov bot commented Oct 13, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (ec14141) 33.50% compared to head (126be39) 33.50%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7431      +/-   ##
==========================================
- Coverage   33.50%   33.50%   -0.01%     
==========================================
  Files        3389     3389              
  Lines      207038   207038              
  Branches    22346    22340       -6     
==========================================
- Hits        69373    69365       -8     
- Misses     136543   136551       +8     
  Partials     1122     1122              

see 3 files with indirect coverage changes

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

@MiloradFilipovic MiloradFilipovic added community Authored by a community member and removed n8n team Authored by the n8n team labels Oct 13, 2023
@MiloradFilipovic MiloradFilipovic merged commit 1fb5166 into n8n-io:master Oct 13, 2023
14 of 23 checks passed
ref: ${{ inputs.branch }}
ref: refs/pull/${{ github.event.pull_request.number }}/merge
Copy link
Contributor

@inga-lovinde inga-lovinde Oct 13, 2023

Choose a reason for hiding this comment

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

Note that, since inputs.branch is seemingly always empty (as I understand, there are no inputs for jobs triggered automatically): the original version passed an empty ref argument to actions/checkout; in this case, actions/checkout falls back to github.context.ref, which is officially defined as PR merge branch already.

So both old and new versions of line 51 have the same effect, and removing line 51 entirely would have the same effect too (namely that PR merge branch is used).

The same applies to line 13, and to line 40 as well, except that units-tests-reusable.yml, as I understand, uses 'master' as the default value if ref is not passed.

So both linting and build were actually done on the correct branch before, the same as passed in this PR to actions/checkout explicitly, as evidenced e.g. by lint and build workflow logs for #7377:

  1 Run actions/checkout@v3.5.3
  2   with:
  3     repository: n8n-io/n8n
  4     token: ***
  5     ssh-strict: true
  6     persist-credentials: true
  7     clean: true
  8     sparse-checkout-cone-mode: true
  9     fetch-depth: 1
 10     lfs: false
 11     submodules: false
 12     set-safe-directory: true
...
 44 Fetching the repository
 45   /usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=1 origin +aa3f5d9d3da7b38e185a2d6355a9add2a0346c08:refs/remotes/pull/7377/merge
...
459 Checking out the ref
460   /usr/bin/git checkout --progress --force refs/remotes/pull/7377/merge

The original issue, #7423, has this explained in more detail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @inga-lovinde thanks much for the thorough write up in the issue! I think you are right and I also saw the fallback gives the same value when testing this but decided to keep an explicit ref since it is easier to see what's going on like this. Hopefully it will help us with debugging in the future without introducing more edgecases.

@janober
Copy link
Member

janober commented Oct 18, 2023

Got released with n8n@1.12.0

elsmr pushed a commit that referenced this pull request Oct 19, 2023
Github issue / Community forum post (link here to close automatically):
#7423

This PR updates reference passed to the `checkout` action by the
`cy-pull-request.ym`. This should fix three existing issues:
- Failing unit tests for external pull requests
- Failing e2e tests for external PRs
- Passing empty `ref` to `lint` job which makes linter run on a wrong
branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Authored by a community member Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants