Repository navigation
Conversation
|
Subscribed to pull request
Generated by CodeMention Warning: The preamble and epilogue options in commentConfiguration are deprecated. Use template instead. |
`--ref` resolved the ref locally with `git rev-parse` and sent only the resulting commit hash, so the branch or tag name never reached the server. Runs dispatched from the CLI were labeled with a 40-character hash, which also filled the website's Git branch filter with hashes. Send branch and tag names through untouched instead. `HEAD`, revision expressions and remote-tracking refs keep being resolved locally, since they either mean nothing in the remote repository or mean something else there. Fixes expo#4311
753d22a to
dc50ac6
Compare
douglowder
left a comment
There was a problem hiding this comment.
Thanks for this change. The branch name in the run list is a good improvement. My main concern is that the CLI now sends only the branch name, so the server can run a different commit than the user has locally (see the comments on line 253). If the API can accept both the pinned commit SHA and the requested ref name, most of these comments go away.
| Log.log(`Using workflow file ${fileName} at ${gitRef}`); | ||
| // Send the branch or tag name the user asked for, so that the server records it as | ||
| // the run's requested git ref instead of a bare commit hash. | ||
| gitRef = resolveWorkflowRunGitRef({ |
There was a problem hiding this comment.
When the CLI sends the branch name, the server uses the latest commit of the branch on the remote, not the local commit. If the local branch has commits that are not pushed, or tracks a remote branch with a different name, the run uses a different commit and the user gets no warning. Before this change, the CLI sent the local SHA, so the run used that exact commit or failed with a clear "commit not found" error. This also goes against the --ref flag description: "the workflow is run from the exact state of the project at the chosen reference".
| Log.log(`Using workflow file ${fileName} at ${gitRef}`); | ||
| // Send the branch or tag name the user asked for, so that the server records it as | ||
| // the run's requested git ref instead of a bare commit hash. | ||
| gitRef = resolveWorkflowRunGitRef({ |
There was a problem hiding this comment.
The server now resolves the branch name 2 times: once in getOrCreateWorkflowRevisionFromGitRefAsync (the workflow YAML) and once in createWorkflowRunFromGitRefAsync (line 368). The input prompts happen between these 2 calls. If someone pushes to the branch during that time, the run can use the YAML from one commit and the source from a different commit. Before this change, both calls used the same SHA.
| Log.log(`Using workflow file ${fileName} at ${gitRef}`); | ||
| // Send the branch or tag name the user asked for, so that the server records it as | ||
| // the run's requested git ref instead of a bare commit hash. | ||
| gitRef = resolveWorkflowRunGitRef({ |
There was a problem hiding this comment.
Possible alternative: WorkflowRun already stores requestedGitRef and gitCommitHash as separate fields. If the mutations accepted both the commit SHA and the requested ref name, the CLI could pin the commit and keep the branch label. Then the prefix and short-name checks (HEAD, @, remote-tracking refs, case, tag/branch with the same name) are not necessary.
| Log.log( | ||
| gitRef === commitSha | ||
| ? `Using workflow file ${fileName} at ${gitRef}` | ||
| : `Using workflow file ${fileName} at ${gitRef} (${commitSha})` |
There was a problem hiding this comment.
This line shows the local SHA, but the server resolves the branch name on its own. If the user has commits that are not pushed, the run uses a different SHA than the one shown here. This can confuse the user when they debug a run.
| if (!symbolicFullName || !prefix) { | ||
| return commitSha; | ||
| } | ||
| const shortName = symbolicFullName.slice(prefix.length); |
There was a problem hiding this comment.
On a filesystem that ignores case (the macOS default), --ref My-Branch can resolve to the loose ref for my-branch, and --symbolic-full-name then returns refs/heads/My-Branch. That matches requestedRef, so the CLI sends My-Branch. GitHub ref names are case-sensitive, so the run fails. Before this change, the CLI sent the SHA and this input worked.
| if (!symbolicFullName || !prefix) { | ||
| return commitSha; | ||
| } | ||
| const shortName = symbolicFullName.slice(prefix.length); |
There was a problem hiding this comment.
If a tag and a branch have the same name (for example release), local git selects refs/tags/release. The short name matches, so the CLI sends release, and the server can resolve it to the branch instead. Sending symbolicFullName (the fully qualified ref) in this case keeps the result the same as local git.
| ['rev-parse', '--symbolic-full-name', '--verify', ref], | ||
| { cwd: projectDir } | ||
| ); | ||
| return output[0].trim() || null; |
There was a problem hiding this comment.
This catch hides all errors, not only the "not a ref" case. If git fails for a different reason (for example a lock or permission error), the CLI sends the SHA again with no log message, and the problem this PR fixes comes back with no sign of why. Can we at least add a Log.debug with the error?
| // the run's requested git ref instead of a bare commit hash. | ||
| gitRef = resolveWorkflowRunGitRef({ | ||
| requestedRef: flags.ref, | ||
| symbolicFullName: await maybeResolveSymbolicFullNameAsync(flags.ref, projectDir), |
There was a problem hiding this comment.
Nit: this starts a second git rev-parse process after the first one. One call, git rev-parse <ref> --symbolic-full-name <ref>, returns both values. Or run the 2 calls in parallel with Promise.all.
|
eas-cli now uses changesets for changelogs (#4580). The required "Check for a changeset" check fails on PRs that edit To update this PR:
The release adds the PR link and the author to the entry. If this PR needs no changelog entry, revert the |
Closes #4311.
Why
workflow:run --ref <branch>resolves the ref withgit rev-parseand sends the resulting commit hash asgitRef, so the branch name never reaches the server.WorkflowRunstoresrequestedGitRefandgitCommitHashseparately — the website's Branch column and its Git-branch filter both readrequestedGitRef— so a CLI-triggered run is labelled with a 40-character hash while the same workflow started from the dashboard's Run workflow button keeps the branch name. The filter fills with hashes and stops being usable.This was flagged during review of the PR that added
--ref(#3203):The local
rev-parsewas only meant to support--ref HEAD; it ended up applying to every ref.What changed
resolveWorkflowRunGitRefpicks what to send. Branch and tag names go through untouched, in short (my-branch) or fully qualified (refs/heads/my-branch) form. Everything else keeps being sent as the locally resolved commit, because it either has no meaning in the remote repository or does not mean the same thing there:--ref--symbolic-full-namemy-branch,feat/x,v1.0,refs/heads/…,refs/tags/…refs/heads|tags/…HEAD,@refs/heads/<current>HEADHEADHEAD~2, raw hashorigin/my-branchrefs/remotes/…heads/my-branchrefs/heads/my-branchHEADdeliberately keeps the old behaviour: it resolves to the default branch server-side, so passing it through would silently run against the wrong branch.rev-parseis still called, so an invalid ref fails exactly as before, and the log line now shows both (at my-branch (abc1234…)).Testing
12 new tests in
run.test.ts. Red/green: replacing the helper body with the oldreturn commitShafails the 5 pass-through cases with the reported bug (Expected "my-branch", received "0e5c2f18…"), while the 7 cases that guardHEAD/detached/expression/hash/remote-tracking behaviour pass in both states.Note:
src/vcs/clients/__tests__/git.test.tscan hit its 5s per-test timeout under parallel load. That is pre-existing — it reproduces on a cleanmainwith these changes stashed.