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.

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

ci: Fix metamaskbot comment test build links #29403

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Dec 20, 2024

Description

Various build links in the metamaskbot PR comment were broken. All links have been fixed except beta (which is trickier to fix, requires more substantial changes to the beta workflows, tracked as #29404). The beta link has been removed until we can fix it. Additionally, the Firefox MMI build link has been removed (this build does not work)

Open in GitHub Codespaces

Related issues

Fixes: #29402

Manual testing steps

  • Test that all build links work

Screenshots/Recordings

N/A

Pre-merge author checklist

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.

@Gudahtt Gudahtt changed the title ci: Fix metamaskbot comment build links ci: Fix metamaskbot comment test build links Dec 20, 2024
- prep-build-mmi
- prep-build-flask
- prep-build-flask-mv2
- prep-build-test
Copy link
Member Author

Choose a reason for hiding this comment

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

These are all required by the prepublish job because it uses the builds persisted to the workspace by these jobs.

Some of these were indirectly guaranteed to be present in the workspace due to them being required by some other listed dependency here. But best not to rely on that.

@Gudahtt Gudahtt force-pushed the fix-metamaskbot-build-links branch 2 times, most recently from 35b4b48 to 6ec646c Compare December 20, 2024 18:17
return `<a href="${url}">${platform}</a>`;
})
.join(', ');
const buildMap = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Specifying the labels and builds declaratively made it easier to apply the same validation and formatting to all of them.

firefox: `${BUILD_LINK_BASE}/builds-mv2/metamask-firefox-${VERSION}.zip`,
},
'builds (beta)': {
chrome: `${BUILD_LINK_BASE}/builds-beta/metamask-flask-chrome-${VERSION}-beta.0.zip`,
Copy link
Member Author

Choose a reason for hiding this comment

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

This link was updated to be "less wrong" (I added the -beta.0 suffix), but it still won't work for two reasons:

  • The beta build isn't even generated on most workflows at the moment
  • Even when it is generated, the number won't always be .0. e.g. it could be beta.1.

To be addressed in the later PR.

@metamaskbot

This comment was marked as outdated.

@metamaskbot

This comment was marked as outdated.

@Gudahtt Gudahtt force-pushed the fix-metamaskbot-build-links branch from 9b86dbc to 440bb95 Compare December 20, 2024 20:02
@@ -167,7 +167,6 @@ workflows:
requires:
- prep-deps
- prep-build-test-flask-mv2:
<<: *main_master_rc_only
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to build it on all PRs in order to include the build link

@metamaskbot
Copy link
Collaborator

Builds ready [be80ecb]
Page Load Metrics (1622 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint25219121561315151
domContentLoaded1452185216029445
load1460190316229847
domInteractive22147453115
backgroundConnect85128189
firstReactRender1694312612
getState44913157
initialActions01000
loadScripts1020138011678139
setupStore65113147
uiStartup16172208187715173
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@@ -783,10 +786,10 @@ jobs:
name: Build extension for testing
command: yarn build:test:flask:mv2
- run:
name: Move test build to 'dist-test-flask' to avoid conflict with production build
name: Move test build to 'dist-test-flask-mv2' to avoid conflict with production build
Copy link
Member Author

Choose a reason for hiding this comment

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

These two changes were copy+paste mistakes in the step description

- store_artifacts:
path: builds-test-flask
- store_artifacts:
path: builds-test-flask-mv2
Copy link
Member Author

@Gudahtt Gudahtt Dec 20, 2024

Choose a reason for hiding this comment

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

This was the main issue with the broken test MV2 build links: we forgot toe store_artifacts step.

Various build links in the `metamaskbot` PR comment were broken. All
links have been fixed except beta (which is trickier to fix, requires
more substantial changes to the beta workflows). Additionally,
validation was added to verify each build link, omitting any that are
not working from the comment (and logging warning).
@Gudahtt Gudahtt force-pushed the fix-metamaskbot-build-links branch from 8e1220a to de0f2dd Compare December 20, 2024 21:01
@metamaskbot
Copy link
Collaborator

Builds ready [de0f2dd]
Page Load Metrics (1857 ± 110 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint89222451773261125
domContentLoaded155224911823225108
load159625071857228110
domInteractive26207615426
backgroundConnect107434199
firstReactRender18100372412
getState786272412
initialActions00000
loadScripts11131995133319594
setupStore675222311
uiStartup176227792130260125
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt Gudahtt marked this pull request as ready for review December 20, 2024 21:54
@Gudahtt Gudahtt requested a review from a team as a code owner December 20, 2024 21:54
@hjetpoluru hjetpoluru self-requested a review December 20, 2024 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: metamaskbot comment broken build links
3 participants