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: pin image versions used in CI so it remains deterministic #28779

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

davidmurdoch
Copy link
Contributor

@davidmurdoch davidmurdoch commented Nov 27, 2024

We shouldn't use image versions in CI that can update all willy-nilly. This will result in non-deterministic test runs.

ubuntu-2404:2024.05.1 is currently the same as ubuntu-2404:2024.05.1 (see https://discuss.circleci.com/t/new-ubuntu-24-04-linux-machine-executor-image/51018#linux-1)

Copy link
Contributor

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.

@davidmurdoch davidmurdoch marked this pull request as ready for review November 27, 2024 17:18
@davidmurdoch davidmurdoch requested review from kumavis and a team as code owners November 27, 2024 17:18
@metamaskbot
Copy link
Collaborator

Builds ready [285f0d8]
Page Load Metrics (1868 ± 135 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint48524141775393189
domContentLoaded143623961828274131
load144724231868281135
domInteractive16130432613
backgroundConnect8102443014
firstReactRender16152493818
getState672232311
initialActions01000
loadScripts10311735131920498
setupStore686262914
uiStartup168229992185357171
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

We've gone back and forth on this a few times. We used to pin browser versions as well, but the team decided at one point that auto-updating made it easier to keep up (and to detect issues with newer browser versions). I see that the browser images are still tags that we'd expect to be updated over time with OS updates.

Still, no objection to pinning, I don't think this is too onerous to keep up with if you see the moving target as causing problems.

@davidmurdoch
Copy link
Contributor Author

We've gone back and forth on this a few times. We used to pin browser versions as well, but the team decided at one point that auto-updating made it easier to keep up (and to detect issues with newer browser versions). I see that the browser images are still tags that we'd expect to be updated over time with OS updates.

Still, no objection to pinning, I don't think this is too onerous to keep up with if you see the moving target as causing problems.

I think it makes sense for our browser versions to auto update because if the extension doesn't work properly on a recent/future browser version it is going to (soon) be failing in production as well [1]. These types of failures are runtime bugs that must be fixed ASAP.

I'm just firmly against having our CI infrastructure changing out from under us; when the CI image is updated the tooling (i.e., node version) is already going to be many versions old anyway; there is just no urgency, so there is no need to create a "CI IS BROKEN!" panic if it auto-updates and things fail on us. Of course current doesn't change very often, but I still appreciate a deterministic CI environment.

[1] We could automate updating our pinned version at every release of a new browser build, Dependabot style. We'd then get the best of both worlds: CI determinism and early detections of browser incompatibilities.

@Gudahtt
Copy link
Member

Gudahtt commented Nov 28, 2024

I see that the browser images are still tags that we'd expect to be updated over time with OS updates.

To clarify this part, I was pointing out that this problem remained with the other executors. The Node version is fixed for them (at least to the minor version), but OS updates would still sporadically be made.

Really we need a setup-node type of step. It does seem egregiously wrong that we're using system node for some of our build steps.

@davidmurdoch
Copy link
Contributor Author

davidmurdoch commented Nov 28, 2024

The Node version is fixed for them (at least to the minor version), but OS updates would still sporadically be made.

Ah, I hadn't even thought about that. Digging into https://circleci.com/developer/images/image/cimg/node its not clear if they'll retag a fully-versioned (e.g., cimg/node:20.17.1-browsers) image with OS updates.

Anyway, you brought up some points I hadn't thought about. I'll mull on it over the long weekend.

@hjetpoluru hjetpoluru self-requested a review December 2, 2024 14:18
@davidmurdoch davidmurdoch added this pull request to the merge queue Dec 2, 2024
Merged via the queue into main with commit 1627181 Dec 3, 2024
84 checks passed
@davidmurdoch davidmurdoch deleted the pin-ci-version branch December 3, 2024 00:07
@github-actions github-actions bot locked and limited conversation to collaborators Dec 3, 2024
@metamaskbot metamaskbot added the release-12.10.0 Issue or pull request that will be included in release 12.10.0 label Dec 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.10.0 Issue or pull request that will be included in release 12.10.0 team-extension-platform
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants