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

Fix racy workflows #1996

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Fix racy workflows #1996

merged 1 commit into from
Mar 7, 2024

Conversation

frelon
Copy link
Contributor

@frelon frelon commented Mar 7, 2024

The merge_commit_sha seems to not always be calculated correctly, try to use github.event.pull_request.head.sha instead.

@davidcassany
Copy link
Contributor

I guess we can simply verify that by checking the toolkit commit matches the branch head. @frelon I am correct?

The merge_commit_sha seems to not always be calculated correctly, try to
use github.event.pull_request.head.sha instead.

Signed-off-by: Fredrik Lönnegren <[email protected]>
@frelon
Copy link
Contributor Author

frelon commented Mar 7, 2024

I guess we can simply verify that by checking the toolkit commit matches the branch head. @frelon I am correct?

You mean the GIT_COMMIT used in the Makefile? Yep, that would be one way to do it.

@codecov-commenter
Copy link

codecov-commenter commented Mar 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.53%. Comparing base (334f4ee) to head (e9a93d1).
Report is 164 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1996   +/-   ##
=======================================
  Coverage   72.53%   72.53%           
=======================================
  Files          76       76           
  Lines        8910     8910           
=======================================
  Hits         6463     6463           
  Misses       1913     1913           
  Partials      534      534           

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

@davidcassany
Copy link
Contributor

MMm there is still something weird in here. See the difference between the Build CLI and build-toolkit jobs.

For unit tests:

  /usr/bin/git checkout --progress --force refs/remotes/pull/1996/merge
  Note: switching to 'refs/remotes/pull/1996/merge'.
  
  You are in 'detached HEAD' state. You can look around, make experimental
  changes and commit them, and you can discard any commits you make in this
  state without impacting any branches by switching back to a branch.
  
  If you want to create a new branch to retain commits you create, you may
  do so (now or later) by using -c with the switch command. Example:
  
    git switch -c <new-branch-name>
  
  Or undo this operation with:
  
    git switch -
  
  Turn off this advice by setting config variable advice.detachedHead to false
  
  HEAD is now at ac6486f Merge e9a93d1b1ea98c8d35e731f0de24a796e854142d into 334f4eee01f0c0654c95ef2062a9f3c69612afaf
/usr/bin/git log -1 --format='%H'
'ac6486f82c5447c6791821717fdd5680679a893c'

e9a93d1 the current head commit of this branch.

and for build-toolkit job:

 /usr/bin/git checkout --progress --force 0894510d641cc5cb2de9d0e1727363933ba90968
  Note: switching to '0894510d641cc5cb2de9d0e1727363933ba90968'.
  
  You are in 'detached HEAD' state. You can look around, make experimental
  changes and commit them, and you can discard any commits you make in this
  state without impacting any branches by switching back to a branch.
  
  If you want to create a new branch to retain commits you create, you may
  do so (now or later) by using -c with the switch command. Example:
  
    git switch -c <new-branch-name>
  
  Or undo this operation with:
  
    git switch -
  
  Turn off this advice by setting config variable advice.detachedHead to false
  
  HEAD is now at 0894510 Merge 6af6e0fc434603bf2fd5fb0a4a22984dc9f1811f into 334f4eee01f0c0654c95ef2062a9f3c69612afaf
/usr/bin/git log -1 --format='%H'
'0894510d641cc5cb2de9d0e1727363933ba90968'

Where 6af6e0f is the previous commit!! 😱

@frelon
Copy link
Contributor Author

frelon commented Mar 7, 2024

Yea, since the pull_request_target is used we will only see the new usage of github.event.pull_request.head.sha when this PR is merged.

@davidcassany
Copy link
Contributor

Yea, since the pull_request_target is used we will only see the new usage of github.event.pull_request.head.sha when this PR is merged.

Oh I see, yes lets give it a try.

@davidcassany davidcassany merged commit a04cecf into rancher:main Mar 7, 2024
15 of 19 checks passed
@frelon frelon deleted the use-head-sha branch March 7, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants