feat(2675): exclude ci build for non-pr #25489

DDDDDanica · 2024-06-24T15:08:18Z

Description

To reduce cost for circle ci and allocate more resources to PR, we remove the circle build for non-pr branches.

Sample test jobs:

When branch name is matching filter as develop, master, release branch starting with Version, here's a test job with mocked branch filter feat/test-circle-ci, be aware that this is for pure testing purpose:

- Condition A:

#`+|feat/test-circle-ci` is for testing purpose
if [[ "$CIRCLE_BRANCH" =~ ^(develop|master|Version-v[0-9]+\.[0-9]+\.[0-9]+|feat/test-circle-ci)$ || -n "$CIRCLE_PULL_REQUESTS" ]]; then
                echo "Branch matches filters. Proceeding with pipeline."
            else
                if [ -z "$CIRCLE_PULL_REQUESTS" ]; then
                    echo "This branch has no associated pull request. Terminating pipeline build."
                    exit 1
                else
                    echo "Branch does not match filters. Terminating pipeline build."
                fi
                exit 1
            fi

Result: (success)

https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/91110/workflows/80415232-2145-4a5d-a994-c89287351fc4/jobs/3384490

- Condition B:

Branch does not have an associated pull request, and does not match the filter, hence running feat/test-circle-ci branch will result in a failed pipeline:

            if [[ "$CIRCLE_BRANCH" =~ ^(develop|master|Version-v[0-9]+\.[0-9]+\.[0-9]+|trigger-ci.*)$ || -n "$CIRCLE_PULL_REQUESTS" ]]; then
                echo "Branch matches filters. Proceeding with pipeline."
            else
                if [ -z "$CIRCLE_PULL_REQUESTS" ]; then
                    echo "This branch has no associated pull request. Terminating pipeline build."
                    exit 1
                else
                    echo "Branch does not match filters. Terminating pipeline build."
                fi
                exit 1
            fi

Result: (failure)

https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/91111/workflows/f084602e-1af6-497f-b192-639861c25e01

- Condition C:

We trigger the build of current branch feature/2675, which doesn't match any name filter but has this current PR associated with it. Hence the pipeline will be triggered.

Result: (success)

https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/91112/workflows/c21314e0-dcab-4813-9031-8e1698524f02/jobs/3384524

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2675

Manual testing steps

Go to this page...

Screenshots/Recordings

Before

After

Condition B

Condition C

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-06-24T15:08:32Z

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.

hjetpoluru

LGTM

Gudahtt · 2024-06-24T18:20:29Z

.circleci/config.yml

@@ -81,6 +81,9 @@ aliases:

 workflows:
  test_and_release:
+    when:
+      not:
+        is: pull_request  # Exclude pull request events


This would break CI on develop and master (which would break releases)

We may also want to allow runs from branches that are manually triggered (not especially important to me but @HowardBraham mentioned this idea in Slack, some people mind find this useful)

Can we look into doing it this way as well?
https://discuss.circleci.com/t/product-launch-override-only-build-prs-setting/47807

@Gudahtt I expanded the script to include develop and master in this commit e3fcfc49

@Gudahtt

We may also want to allow runs from branches that are manually triggered (not especially important to me but @HowardBraham mentioned this idea in Slack, some people mind find this useful)

Can we target in a different ticket, and use an agreed scheme like trigger-CI-feature-123-test where we always have trigger-CI as prefix for branches who needs to have a pipeline before creating a PR?

@HowardBraham Thanks for the information, but I was a bit lost there. Can you elaborate more on this propsal?
my understanding is, in the thread an API is suggested to config the setting. But we can use a schema in yml file to target the branch segmentation. wdyt?

Yeah, I agree it's confusing and I definitely don't completely understand it either. Basically there exist options in the API that are not available in the YML.

I thought about that trigger-CI thing, yeah can we do that for now?

can you explain to me again about the trigger-CI thing, is this related to triggering job manually in branch page in circle ci?

I'm not sure there's a way to configure the manual trigger thing in config.yml, I think the only way to do it is with that CircleCI API.

But if you make the pattern /^(master|develop|trigger-CI-.*)$/ it should at least run automatically on those branches.

Also it's a little hard to test, but I don't think it's going to run on master or develop as written. I think it will only run on Pull Requests.

Thanks Howard for suggestions, i configured to trigger it in each run and integrated the pattern you suggested~

codecov · 2024-07-10T14:35:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.77%. Comparing base (5ee57a6) to head (8abf362).
Report is 29 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #25489      +/-   ##
===========================================
- Coverage    69.77%   69.77%   -0.00%     
===========================================
  Files         1376     1378       +2     
  Lines        48403    48578     +175     
  Branches     13348    13394      +46     
===========================================
+ Hits         33773    33895     +122     
- Misses       14630    14683      +53

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

metamaskbot · 2024-07-11T01:40:20Z

Builds ready [013018f]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (65 ± 8 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	62	141	92	17	8
		domContentLoaded	9	56	24	10	5
		load	41	113	65	16	8
		domInteractive	9	56	23	10	5

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

sonarqubecloud · 2024-07-11T02:02:30Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-07-11T11:08:46Z

Builds ready [8abf362]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (161 ± 189 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	64	130	102	18	8
		domContentLoaded	9	58	26	14	7
		load	41	1873	161	393	189
		domInteractive	9	58	26	14	7

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

HowardBraham · 2024-07-11T22:18:16Z

The solution in this PR is a little cumbersome. I implemented it the other way, as discussed in this article:
https://support.circleci.com/hc/en-us/articles/22357021358875-Build-on-Pull-Requests-and-all-commits

I turned on this switch in our CircleCI settings:

For our future records, the API call I ran was:

curl -X PUT \
--header "Circle-Token: <token>" \
--header "Accept: application/json" \
--header "Content-Type: application/json" \
--data '{"feature_flags":{"pr-only-branch-overrides": "develop, master, trigger-ci.*, Version-v[0-9]+.[0-9]+.[0-9]+"}}' \
'https://circleci.com/api/v1.1/project/github/MetaMask/metamask-extension/settings'

This is the result of my tests on different branch names, notice the Not Run:

@DDDDDanica @Gudahtt please do try some independent tests on this, but I think it's fully working now without this PR.

pedronfigueiredo · 2024-07-12T10:00:24Z

.circleci/config.yml

+          command: |
+            #!/bin/bash
+            # Validate if branch is master, develop or release branch
+            if [[ "$CIRCLE_BRANCH" =~ ^(develop|master|Version-v[0-9]+\.[0-9]+\.[0-9]+|trigger-ci.*)$ || -n "$CIRCLE_PULL_REQUESTS" ]]; then


[nit] can you include a mention of trigger-ci.* in the comment?

pedronfigueiredo · 2024-07-12T10:03:43Z

.circleci/config.yml

@@ -423,12 +451,6 @@ jobs:
            #!/bin/bash

            GH_LABEL=team-mmi
-            if [ -z "$CIRCLE_PULL_REQUESTS" ]; then
-              echo "Skipping tag check; this is not a PR."
-              echo "false" > ./RUN_MMI_OPTIONAL


doesn't the fact that we don't assign ./RUN_MMI_OPTIONAL anymore substantially change the script?

DDDDDanica · 2024-07-12T11:37:13Z

Hey @HowardBraham thanks for providing this solution. Unfortunately i cannot access the settings page you shared with. But this solution seems neat ! And then i checked the CURL you sent and it seems like this CURL was doing the opposite of what we are looking for? As now it is not running for branch trigger-ci.* or Version-C and running for other branches only. is something I'm missing here?

HowardBraham · 2024-07-13T00:18:10Z

@DDDDDanica I believe the CURL call is saying:
Only run CircleCI on pull requests except use the overrides
{"pr-only-branch-overrides": "develop, master, trigger-ci.*, Version-v[0-9]+.[0-9]+.[0-9]+"}

So run on branches develop, master, trigger-ci.*, and Versions

DDDDDanica · 2024-07-13T02:56:21Z

@HowardBraham the script seems correct, my bad, the branch I was testing was not named properly, and i believe this approach is the easiest one. Tested it locally as well and all works well.
@danjm and @Gudahtt If this approach is also acceptable with you, we can go ahead and close this PR & Ticket

danjm · 2024-07-19T09:00:00Z

It does seem like Howard's solution is working

DDDDDanica · 2024-07-19T23:39:53Z

Close because this has been implemented by @HowardBraham with a cleaner solution

DDDDDanica requested review from kumavis and a team as code owners June 24, 2024 15:08

DDDDDanica requested a review from danjm June 24, 2024 15:08

pedronfigueiredo previously approved these changes Jun 24, 2024

View reviewed changes

metamaskbot added the team-extension-platform label Jun 24, 2024

hjetpoluru requested review from hjetpoluru June 24, 2024 16:11

hjetpoluru previously approved these changes Jun 24, 2024

View reviewed changes

Gudahtt requested changes Jun 24, 2024

View reviewed changes

DDDDDanica dismissed stale reviews from hjetpoluru and pedronfigueiredo via c6f2990 June 25, 2024 21:51

DDDDDanica force-pushed the feature/2675 branch 3 times, most recently from 5142f01 to 29c3e29 Compare June 28, 2024 16:48

DDDDDanica force-pushed the feature/2675 branch from 29c3e29 to d3fed6b Compare July 10, 2024 14:04

feat(2675): exclude ci build for non-pr

bfa2924

DDDDDanica force-pushed the feature/2675 branch 3 times, most recently from e578631 to 013018f Compare July 11, 2024 01:09

feat(2675): expanded jobs with develop and master

8abf362

DDDDDanica force-pushed the feature/2675 branch from 013018f to 8abf362 Compare July 11, 2024 02:00

HowardBraham mentioned this pull request Jul 11, 2024

Disable CI on the l10n_crowdin_action branch #25770

Closed

9 tasks

pedronfigueiredo reviewed Jul 12, 2024

View reviewed changes

DDDDDanica closed this Jul 19, 2024

github-actions bot locked and limited conversation to collaborators Jul 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(2675): exclude ci build for non-pr #25489

feat(2675): exclude ci build for non-pr #25489

DDDDDanica commented Jun 24, 2024 •

edited

Loading

github-actions bot commented Jun 24, 2024

hjetpoluru left a comment

Gudahtt Jun 24, 2024

Gudahtt Jun 24, 2024

HowardBraham Jun 24, 2024

DDDDDanica Jun 27, 2024

DDDDDanica Jun 27, 2024

DDDDDanica Jun 27, 2024

HowardBraham Jun 28, 2024

DDDDDanica Jun 28, 2024 •

edited

Loading

HowardBraham Jun 28, 2024

DDDDDanica Jul 11, 2024

codecov bot commented Jul 10, 2024 •

edited

Loading

metamaskbot commented Jul 11, 2024

sonarqubecloud bot commented Jul 11, 2024

metamaskbot commented Jul 11, 2024

HowardBraham commented Jul 11, 2024

pedronfigueiredo Jul 12, 2024

pedronfigueiredo Jul 12, 2024

DDDDDanica commented Jul 12, 2024 •

edited

Loading

HowardBraham commented Jul 13, 2024

DDDDDanica commented Jul 13, 2024

danjm commented Jul 19, 2024

DDDDDanica commented Jul 19, 2024

feat(2675): exclude ci build for non-pr #25489

feat(2675): exclude ci build for non-pr #25489

Conversation

DDDDDanica commented Jun 24, 2024 • edited Loading

Description

Sample test jobs:

- Condition A:

Result: (success)

- Condition B:

Result: (failure)

- Condition C:

Result: (success)

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jun 24, 2024

hjetpoluru left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DDDDDanica Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 10, 2024 • edited Loading

Codecov Report

metamaskbot commented Jul 11, 2024

sonarqubecloud bot commented Jul 11, 2024

Quality Gate passed

metamaskbot commented Jul 11, 2024

HowardBraham commented Jul 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DDDDDanica commented Jul 12, 2024 • edited Loading

HowardBraham commented Jul 13, 2024

DDDDDanica commented Jul 13, 2024

danjm commented Jul 19, 2024

DDDDDanica commented Jul 19, 2024

DDDDDanica commented Jun 24, 2024 •

edited

Loading

DDDDDanica Jun 28, 2024 •

edited

Loading

codecov bot commented Jul 10, 2024 •

edited

Loading

DDDDDanica commented Jul 12, 2024 •

edited

Loading