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.

Sign up for GitHub

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

Jump to bottom

ci: make git-diff-develop work for PRs from foreign repos #27268

Merged
merged 5 commits into from
Oct 8, 2024
Merged

ci: make git-diff-develop work for PRs from foreign repos #27268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ef03847
ci: make git-diff-develop work for PRs from foreign repos
legobeat Sep 19, 2024
91ce41d
ci: git-diff-develop does not depend on prep-deps
legobeat Sep 19, 2024
978eae7
ci: get-changed-files-with-git-diff: stable branch names
legobeat Sep 19, 2024
04e5f48
prettier
legobeat Sep 19, 2024
c554b95
back-ported changes from #27548
HowardBraham Oct 8, 2024
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
14 changes: 5 additions & 9 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,9 @@ workflows:
- prep-deps
- get-changed-files-with-git-diff:
filters:
branches:
ignore:
- master
requires:
- prep-deps
branches:
ignore:
- master
- test-deps-audit:
requires:
- prep-deps
Expand Down Expand Up @@ -360,11 +358,10 @@ workflows:
value: << pipeline.git.branch >>
jobs:
- prep-deps
- get-changed-files-with-git-diff:
requires:
- prep-deps
- get-changed-files-with-git-diff
- validate-locales-only:
requires:
- prep-deps
- get-changed-files-with-git-diff
- test-lint:
requires:
Expand Down Expand Up @@ -501,7 +498,6 @@ jobs:
- run: sudo corepack enable
- attach_workspace:
at: .
- gh/install
- run:
name: Get changed files with git diff
command: npx tsx .circleci/scripts/git-diff-develop.ts
Expand Down
82 changes: 50 additions & 32 deletions .circleci/scripts/git-diff-develop.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,42 @@
import { hasProperty } from '@metamask/utils';
import { exec as execCallback } from 'child_process';
import fs from 'fs';
import path from 'path';
import { promisify } from 'util';

const exec = promisify(execCallback);

// The CIRCLE_PR_NUMBER variable is only available on forked Pull Requests
const PR_NUMBER =
process.env.CIRCLE_PR_NUMBER ||
process.env.CIRCLE_PULL_REQUEST?.split('/').pop();

const MAIN_BRANCH = 'develop';
const SOURCE_BRANCH = `refs/pull/${PR_NUMBER}/head`;

const CHANGED_FILES_DIR = 'changed-files';

type PRInfo = {
base: {
ref: string;
};
body: string;
};

/**
* Get the target branch for the given pull request.
* Get JSON info about the given pull request
*
* @returns The name of the branch targeted by the PR.
* @returns JSON info from GitHub
*/
async function getBaseRef(): Promise<string | null> {
if (!process.env.CIRCLE_PULL_REQUEST) {
async function getPrInfo(): Promise<PRInfo | null> {
if (!PR_NUMBER) {
return null;
}

// We're referencing the CIRCLE_PULL_REQUEST environment variable within the script rather than
// passing it in because this makes it easier to use Bash parameter expansion to extract the
// PR number from the URL.
const result = await exec(`gh pr view --json baseRefName "\${CIRCLE_PULL_REQUEST##*/}" --jq '.baseRefName'`);
const baseRef = result.stdout.trim();
return baseRef;
return await (
await fetch(
`https://api.github.com/repos/${process.env.CIRCLE_PROJECT_USERNAME}/${process.env.CIRCLE_PROJECT_REPONAME}/pulls/${PR_NUMBER}`,
)
).json();
}

/**
Expand All @@ -34,8 +47,10 @@ async function getBaseRef(): Promise<string | null> {
*/
async function fetchWithDepth(depth: number): Promise<boolean> {
try {
await exec(`git fetch --depth ${depth} origin develop`);
await exec(`git fetch --depth ${depth} origin ${process.env.CIRCLE_BRANCH}`);
await exec(`git fetch --depth ${depth} origin "${MAIN_BRANCH}"`);
await exec(
`git fetch --depth ${depth} origin "${SOURCE_BRANCH}:${SOURCE_BRANCH}"`,
);
return true;
} catch (error: unknown) {
console.error(`Failed to fetch with depth ${depth}:`, error);
Expand All @@ -59,18 +74,16 @@ async function fetchUntilMergeBaseFound() {
await exec(`git merge-base origin/HEAD HEAD`);
return;
} catch (error: unknown) {
if (
error instanceof Error &&
hasProperty(error, 'code') &&
error.code === 1
) {
console.error(`Error 'no merge base' encountered with depth ${depth}. Incrementing depth...`);
if (error instanceof Error && 'code' in error) {
console.error(
`Error 'no merge base' encountered with depth ${depth}. Incrementing depth...`,
);
} else {
throw error;
}
}
}
await exec(`git fetch --unshallow origin develop`);
await exec(`git fetch --unshallow origin "${MAIN_BRANCH}"`);
}

/**
Expand All @@ -82,9 +95,11 @@ async function fetchUntilMergeBaseFound() {
*/
async function gitDiff(): Promise<string> {
await fetchUntilMergeBaseFound();
const { stdout: diffResult } = await exec(`git diff --name-only origin/HEAD...${process.env.CIRCLE_BRANCH}`);
const { stdout: diffResult } = await exec(
Copy link
Member

@Gudahtt Gudahtt Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm noticing a problem here: we're comparing against the base branch, not the base commit. And similarly, we're comparing the current branch, not the current commit. This is problematic because the results are now dependent upon the exact timing of when this runs, and they're comparing changes made to develop that aren't in the target branch yet.

We're using this diff as a measure of what has changed in the PR, but because we're not comparing with the correct commit, we're going to get skewed results (as changes are made to develop, or as develop is merged into the PR, etc.). This problem is in the original implementation as well though, not introduced here.

I will follow up on this separately. We have fixed this same problem already elsewhere, we can fix it here too without much effort.

Edit: On second thought, it's just the reference to the current branch that is problematic, because when a later commit is made it would impact the CI run of prior commits.

Using origin/develop as the point of comparison is less problematic. In theory it still makes this less deterministic, but maybe not in a way that matters (e.g. it could result in the diff getting smaller if some portion of the branch was merged as a separate PR, but the results would be equally "correct" in a sense).

Copy link
Member

Choose a reason for hiding this comment

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

Given that this is just used to run modified tests additional times, I'm having a hard time imagining a scenario where this has a negative impact, despite it being technically incorrect. I'm thinking maybe this is fine, not worth fixing for now.

`git diff --name-only "origin/HEAD...${SOURCE_BRANCH}"`,
);
if (!diffResult) {
throw new Error('Unable to get diff after full checkout.');
throw new Error('Unable to get diff after full checkout.');
}
return diffResult;
}
Expand All @@ -99,30 +114,33 @@ async function storeGitDiffOutput() {
// Create the directory
// This is done first because our CirleCI config requires that this directory is present,
// even if we want to skip this step.
const outputDir = 'changed-files';
fs.mkdirSync(outputDir, { recursive: true });
fs.mkdirSync(CHANGED_FILES_DIR, { recursive: true });

console.log(`Determining whether this run is for a PR targetting ${MAIN_BRANCH}`)
if (!process.env.CIRCLE_PULL_REQUEST) {
console.log("Not a PR, skipping git diff");
console.log(
`Determining whether this run is for a PR targeting ${MAIN_BRANCH}`,
);
if (!PR_NUMBER) {
console.log('Not a PR, skipping git diff');
return;
}

const baseRef = await getBaseRef();
if (baseRef === null) {
console.log("Not a PR, skipping git diff");
const prInfo = await getPrInfo();

const baseRef = prInfo?.base.ref;
if (!baseRef) {
console.log('Not a PR, skipping git diff');
return;
} else if (baseRef !== MAIN_BRANCH) {
console.log(`This is for a PR targeting '${baseRef}', skipping git diff`);
return;
}

console.log("Attempting to get git diff...");
console.log('Attempting to get git diff...');
const diffOutput = await gitDiff();
console.log(diffOutput);

// Store the output of git diff
const outputPath = path.resolve(outputDir, 'changed-files.txt');
const outputPath = path.resolve(CHANGED_FILES_DIR, 'changed-files.txt');
fs.writeFileSync(outputPath, diffOutput.trim());

console.log(`Git diff results saved to ${outputPath}`);
Expand Down
Loading