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

legobeat · 2024-09-19T04:00:29Z

The gh CLI requires authentication, even if the API endpoints do not. We can just fetch the PR URL directly without shelling out.

Fixes CI error.

Also removes dependency on prep-deps step by removing dependency on external module.

github-actions · 2024-09-19T04:00:41Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

legobeat · 2024-09-19T04:50:45Z

.circleci/scripts/git-diff-develop.ts

@@ -61,7 +56,7 @@ async function fetchUntilMergeBaseFound() {
    } catch (error: unknown) {
      if (
        error instanceof Error &&
-        hasProperty(error, 'code') &&
+        Object.hasOwnProperty.call(error, 'code') &&


hasProperty is the only function being required from an external module. By inlining it, we can remove the dependency on prep-deps in CI.

HowardBraham · 2024-09-19T06:36:55Z

Did you test this with a changed spec file to see if it works?

legobeat · 2024-09-19T07:33:20Z

Did you test this with a changed spec file to see if it works?

Will this do? Also covering the locale-file checking (usually only running on crowdin prs), which also depends on the job.

HowardBraham

I think this is going to break non-forks, see this: https://support.circleci.com/hc/en-us/articles/360047521451-Why-is-CIRCLE-PR-NUMBER-empty

CIRCLE_PR_NUMBER is only defined in forks, yeah it's super weird.

.circleci/scripts/git-diff-develop.ts

Gudahtt

Looks good! Once the PR_NUMBER issue is resolved, I can approve. I like how Howard solved that issue in #27548

Gudahtt · 2024-10-08T17:21:43Z

This partially fixes #27135

@itsyoboieltr is working on getting SonarCloud to run on forks, which is the other task in that issue.

The gh API requires authentication, even if the API endpoints do not. We can just fetch the PR URL directly without shelling out.

Gudahtt

LGTM!

Gudahtt · 2024-10-08T20:30:33Z

.circleci/scripts/git-diff-develop.ts

@@ -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}`);


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

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.

Gudahtt · 2024-10-08T20:53:48Z

I'm force-merging this to get around the failing Sonar Cloud check, which is broken for forks. @legobeat I would have liked to wait until you had a chance to review that last commit, but it's fairly similar to Howard's other PR (which you had approved) so I'm assuming you're good with this approach. My apologies if that assumption is incorrect.

legobeat · 2024-10-08T23:04:11Z

@Gudahtt & @HowardBraham : All good, thanks to both of you for straightening this out and getting it in!

metamaskbot · 2024-10-21T19:11:06Z

No release label on PR. Adding release label release-12.6.0 on PR, as PR was added to branch 12.6.0 when release was cut.

legobeat changed the title ~~ci fix git diff~~ ci: make git-diff-develop work for PRs from foreign repos Sep 19, 2024

metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Sep 19, 2024

legobeat force-pushed the ci-fix-git-diff branch from b316eca to 3e6d1ba Compare September 19, 2024 04:01

legobeat added the team-lavamoat label Sep 19, 2024

legobeat force-pushed the ci-fix-git-diff branch 3 times, most recently from 74bfede to c38a9c4 Compare September 19, 2024 04:48

legobeat commented Sep 19, 2024

View reviewed changes

legobeat force-pushed the ci-fix-git-diff branch from e051da3 to 167babe Compare September 19, 2024 05:12

legobeat marked this pull request as ready for review September 19, 2024 05:18

legobeat requested review from kumavis and a team as code owners September 19, 2024 05:18

legobeat force-pushed the ci-fix-git-diff branch from 167babe to c8a56aa Compare September 19, 2024 05:18

legobeat requested review from HowardBraham and bergeron September 19, 2024 05:19

legobeat force-pushed the ci-fix-git-diff branch from c8a56aa to 256a561 Compare September 19, 2024 05:21

This was referenced Sep 19, 2024

[ci-trigger] git-diff-develop dependent-jobs (#27268) #27270

Draft

chore(ci): disable retries of failing e2e jobs #24972

Open

legobeat requested a review from seaona September 19, 2024 08:04

legobeat force-pushed the ci-fix-git-diff branch 3 times, most recently from 3827bf4 to 58e0bf9 Compare September 23, 2024 22:03

HowardBraham requested changes Sep 26, 2024

View reviewed changes

HowardBraham mentioned this pull request Oct 2, 2024

ci: followup to CircleCI Sentry reporting #27548

Merged

7 tasks

Gudahtt reviewed Oct 8, 2024

View reviewed changes

.circleci/scripts/git-diff-develop.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Oct 8, 2024

View reviewed changes

.circleci/scripts/git-diff-develop.ts Outdated Show resolved Hide resolved

Gudahtt reviewed Oct 8, 2024

View reviewed changes

legobeat and others added 5 commits October 8, 2024 13:13

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

ef03847

The gh API requires authentication, even if the API endpoints do not. We can just fetch the PR URL directly without shelling out.

ci: git-diff-develop does not depend on prep-deps

91ce41d

ci: get-changed-files-with-git-diff: stable branch names

978eae7

prettier

04e5f48

back-ported changes from MetaMask#27548

c554b95

HowardBraham force-pushed the ci-fix-git-diff branch from 58e0bf9 to c554b95 Compare October 8, 2024 20:14

Gudahtt approved these changes Oct 8, 2024

View reviewed changes

HowardBraham approved these changes Oct 8, 2024

View reviewed changes

HowardBraham enabled auto-merge October 8, 2024 20:35

Gudahtt disabled auto-merge October 8, 2024 20:45

Gudahtt merged commit b5b8b8f into MetaMask:develop Oct 8, 2024
75 of 77 checks passed

github-actions bot locked and limited conversation to collaborators Oct 8, 2024

metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

legobeat commented Sep 19, 2024 •

edited

Loading

github-actions bot commented Sep 19, 2024

legobeat Sep 19, 2024

HowardBraham commented Sep 19, 2024

legobeat commented Sep 19, 2024 •

edited

Loading

HowardBraham left a comment

Gudahtt left a comment •

edited

Loading

Gudahtt commented Oct 8, 2024

Gudahtt left a comment

Gudahtt Oct 8, 2024 •

edited

Loading

Gudahtt Oct 8, 2024

Gudahtt commented Oct 8, 2024 •

edited

Loading

legobeat commented Oct 8, 2024

metamaskbot commented Oct 21, 2024

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

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

Conversation

legobeat commented Sep 19, 2024 • edited Loading

github-actions bot commented Sep 19, 2024

legobeat Sep 19, 2024

Choose a reason for hiding this comment

HowardBraham commented Sep 19, 2024

legobeat commented Sep 19, 2024 • edited Loading

HowardBraham left a comment

Choose a reason for hiding this comment

Gudahtt left a comment • edited Loading

Choose a reason for hiding this comment

Gudahtt commented Oct 8, 2024

Gudahtt left a comment

Choose a reason for hiding this comment

Gudahtt Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Gudahtt Oct 8, 2024

Choose a reason for hiding this comment

Gudahtt commented Oct 8, 2024 • edited Loading

legobeat commented Oct 8, 2024

metamaskbot commented Oct 21, 2024

legobeat commented Sep 19, 2024 •

edited

Loading

legobeat commented Sep 19, 2024 •

edited

Loading

Gudahtt left a comment •

edited

Loading

Gudahtt Oct 8, 2024 •

edited

Loading

Gudahtt commented Oct 8, 2024 •

edited

Loading