Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ This is the log of notable changes to EAS CLI and related packages.

### 🐛 Bug fixes

- [eas-cli] Send the branch or tag name to `eas workflow:run --ref`, instead of resolving it to a commit hash locally, so that runs are labeled with the ref that was requested. ([#4355](https://github.com/expo/eas-cli/pull/4355) by [@dennytosp](https://github.com/dennytosp))
- [build-tools] Report early Argent server exits during simulator startup instead of waiting for the full readiness timeout. ([#4454](https://github.com/expo/eas-cli/pull/4454) by [@sjkim-expo](https://github.com/sjkim-expo))
- [eas-cli] Stop spinners from printing a new line on every frame after the terminal gets narrower. ([#4470](https://github.com/expo/eas-cli/pull/4470) by [@brentvatne](https://github.com/brentvatne))
- [eas-cli] Exclude time waiting for simulator concurrency from the session startup timeout and show queue progress. ([#4411](https://github.com/expo/eas-cli/pull/4411) by [@szdziedzic](https://github.com/szdziedzic))
Expand Down
120 changes: 119 additions & 1 deletion packages/eas-cli/src/commands/workflow/__tests__/run.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {
parseJsonInputs,
parseWorkflowInputsFromYaml,
} from '../../../commandUtils/workflow/inputs';
import { resolveWorkflowRunSshInput } from '../run';
import { resolveWorkflowRunGitRef, resolveWorkflowRunSshInput } from '../run';

describe(resolveWorkflowRunSshInput, () => {
it('is null when --ssh is not set', () => {
Expand All @@ -29,6 +29,124 @@ describe(resolveWorkflowRunSshInput, () => {
});
});

describe(resolveWorkflowRunGitRef, () => {
const commitSha = '0e5c2f18c7e4d0e2f5a1b3c4d5e6f708192a3b4c';

it('passes a branch name through instead of the commit it points at', () => {
expect(
resolveWorkflowRunGitRef({
requestedRef: 'my-branch',
symbolicFullName: 'refs/heads/my-branch',
commitSha,
})
).toBe('my-branch');
});

it('passes a fully qualified branch name through', () => {
expect(
resolveWorkflowRunGitRef({
requestedRef: 'refs/heads/my-branch',
symbolicFullName: 'refs/heads/my-branch',
commitSha,
})
).toBe('refs/heads/my-branch');
});

it('passes a branch name containing slashes through', () => {
expect(
resolveWorkflowRunGitRef({
requestedRef: 'feat/my-branch',
symbolicFullName: 'refs/heads/feat/my-branch',
commitSha,
})
).toBe('feat/my-branch');
});

it('passes a tag name through', () => {
expect(
resolveWorkflowRunGitRef({
requestedRef: 'v1.0.0',
symbolicFullName: 'refs/tags/v1.0.0',
commitSha,
})
).toBe('v1.0.0');
});

it('passes a fully qualified tag name through', () => {
expect(
resolveWorkflowRunGitRef({
requestedRef: 'refs/tags/v1.0.0',
symbolicFullName: 'refs/tags/v1.0.0',
commitSha,
})
).toBe('refs/tags/v1.0.0');
});

// HEAD resolves to the default branch server-side, so it has to stay a commit.
it('resolves HEAD to a commit', () => {
expect(
resolveWorkflowRunGitRef({
requestedRef: 'HEAD',
symbolicFullName: 'refs/heads/main',
commitSha,
})
).toBe(commitSha);
});

it('resolves @ to a commit', () => {
expect(
resolveWorkflowRunGitRef({
requestedRef: '@',
symbolicFullName: 'refs/heads/main',
commitSha,
})
).toBe(commitSha);
});

it('resolves a detached HEAD to a commit', () => {
expect(
resolveWorkflowRunGitRef({
requestedRef: 'HEAD',
symbolicFullName: 'HEAD',
commitSha,
})
).toBe(commitSha);
});

it('resolves a revision expression to a commit', () => {
expect(
resolveWorkflowRunGitRef({ requestedRef: 'HEAD~2', symbolicFullName: null, commitSha })
).toBe(commitSha);
});

it('resolves a commit hash to a commit', () => {
expect(
resolveWorkflowRunGitRef({ requestedRef: '0e5c2f1', symbolicFullName: null, commitSha })
).toBe(commitSha);
});

// `origin/my-branch` is a local name for a remote branch; the server would not resolve it.
it('resolves a remote-tracking ref to a commit', () => {
expect(
resolveWorkflowRunGitRef({
requestedRef: 'origin/my-branch',
symbolicFullName: 'refs/remotes/origin/my-branch',
commitSha,
})
).toBe(commitSha);
});

it('resolves a partially qualified branch name to a commit', () => {
expect(
resolveWorkflowRunGitRef({
requestedRef: 'heads/my-branch',
symbolicFullName: 'refs/heads/my-branch',
commitSha,
})
).toBe(commitSha);
});
});

describe('parseInputs', () => {
it('should parse single key=value pair', () => {
const inputs = parseInputs(['key=value']);
Expand Down
73 changes: 70 additions & 3 deletions packages/eas-cli/src/commands/workflow/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,62 @@ export function resolveWorkflowRunSshInput({
return ssh ? { idleTimeoutSeconds } : null;
}

const PASSTHROUGH_REF_PREFIXES = ['refs/heads/', 'refs/tags/'];

/**
* Pick the value to send as `gitRef` when running a workflow from `--ref`.
*
* The server resolves the ref itself and records it as the run's `requestedGitRef`,
* which is what the website shows in the "Branch" column and offers in the Git branch
* filter. Resolving the ref to a commit locally before dispatching therefore throws
* away the branch or tag name the user asked for.
*
* Branch and tag names are passed through untouched, whether they are given 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: `HEAD` and `@` resolve to the
* default branch server-side, revision expressions such as `HEAD~2` are not refs at all,
* and remote-tracking refs like `origin/my-branch` are local names for remote branches.
*/
export function resolveWorkflowRunGitRef({
requestedRef,
symbolicFullName,
commitSha,
}: {
requestedRef: string;
symbolicFullName: string | null;
commitSha: string;
}): string {
const prefix = PASSTHROUGH_REF_PREFIXES.find(candidate =>
symbolicFullName?.startsWith(candidate)
);
if (!symbolicFullName || !prefix) {
return commitSha;
}
const shortName = symbolicFullName.slice(prefix.length);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

return requestedRef === shortName || requestedRef === symbolicFullName ? requestedRef : commitSha;
}

/**
* Full ref name (e.g. `refs/heads/my-branch`) that `ref` points at, or null when it does
* not name a ref (a commit hash, a revision expression, or a detached `HEAD`).
*/
async function maybeResolveSymbolicFullNameAsync(
ref: string,
projectDir: string
): Promise<string | null> {
try {
const { output } = await spawnAsync(
'git',
['rev-parse', '--symbolic-full-name', '--verify', ref],
{ cwd: projectDir }
);
return output[0].trim() || null;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

} catch {
return null;
}
}

export default class WorkflowRun extends EasCommand {
static override description =
'run an EAS workflow. The entire local project directory will be packaged and uploaded to EAS servers for the workflow run, unless the --ref flag is used.';
Expand Down Expand Up @@ -184,15 +240,26 @@ export default class WorkflowRun extends EasCommand {
// Run from git ref
const fileName = path.basename(args.file);
// Find the real commit, make sure the ref is valid
gitRef = (
const commitSha = (
await spawnAsync('git', ['rev-parse', flags.ref], {
cwd: projectDir,
})
).output[0].trim();
if (!gitRef) {
if (!commitSha) {
throw new Error('Failed to resolve git 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({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

requestedRef: flags.ref,
symbolicFullName: await maybeResolveSymbolicFullNameAsync(flags.ref, projectDir),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

commitSha,
});
Log.log(
gitRef === commitSha
? `Using workflow file ${fileName} at ${gitRef}`
: `Using workflow file ${fileName} at ${gitRef} (${commitSha})`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

);
let revisionResult: WorkflowRevision | undefined;
try {
revisionResult = await WorkflowRevisionMutation.getOrCreateWorkflowRevisionFromGitRefAsync(
Expand Down
Loading