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

feat(IDX): don't rely on bazel cache for large test deps #2752

Merged
merged 8 commits into from
Nov 22, 2024
Merged

Conversation

nmattia
Copy link
Contributor

@nmattia nmattia commented Nov 21, 2024

This changes the system tests to not assume that big dependencies like ic-os images are present in the Bazel cache.

In particular, this adds an upload script (upload_systest_dep.sh) which is run by the "local" (non-farm-host) test runner to ensure that the images have been "stored". Currently the meaning of "stored" means that the images are in the bazel-remote cache, though this is now an implementation detail, encapsulated in the upload_systest_dep script.

This ensures that images are always ready when the tests are run, even if the remote bazel cache was previously prune. This also gives us more flexibility for implementing the Bazel cache differently. This also simplifies the bazel build by not requiring both a URL & a sha256 file for images; instead the file can be propagated directly.

@github-actions github-actions bot added the feat label Nov 21, 2024
This changes the system tests to not assume that big dependencies like
ic-os images are present in the Bazel cache.

In particular, this adds an upload script (`upload_systest_dep.sh`)
which is run by the "local" (non-farm-host) test runner to ensure that
the images have been "stored". Currently the meaning of "stored" means
that the images are in the `bazel-remote` cache, though this is now an
implementation detail, encapsulated in the `upload_systest_dep` script.

This ensures that images are always ready when the tests are run, even
if the remote bazel cache was previously prune. This also gives us more
flexibility for implementing the Bazel cache differently. This also
simplifies the bazel build by not requiring both a URL & a sha256 file
for images; instead the file can be propagated directly.
Copy link
Collaborator

@basvandijk basvandijk left a comment

Choose a reason for hiding this comment

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

Very nice! Just one request to simplify the upload script.

Do you have some numbers on how this affects the system-test execution times?

bazel/upload_systest_dep.sh Outdated Show resolved Hide resolved
bazel/upload_systest_dep.sh Outdated Show resolved Hide resolved
bazel/upload_systest_dep.sh Outdated Show resolved Hide resolved
rs/tests/boundary_nodes/BUILD.bazel Outdated Show resolved Hide resolved
@nmattia nmattia marked this pull request as ready for review November 22, 2024 12:17
@nmattia nmattia requested review from a team as code owners November 22, 2024 12:17
@nmattia nmattia enabled auto-merge November 22, 2024 13:44
@nmattia nmattia added this pull request to the merge queue Nov 22, 2024
Merged via the queue into master with commit e9f61b8 Nov 22, 2024
24 checks passed
@nmattia nmattia deleted the nm-cas-push branch November 22, 2024 14:41
nmattia added a commit that referenced this pull request Nov 22, 2024
A previous [change](#2752) zealously
changed more environment variables than necessary. The
`ENV_DEPS__DEV_SETUPOS_IMG_TAR_ZST` should not point to a CAS URL but to
the image itself.
github-merge-queue bot pushed a commit that referenced this pull request Nov 22, 2024
A previous [change](#2752) zealously
changed more environment variables than necessary. The
`ENV_DEPS__DEV_SETUPOS_IMG_TAR_ZST` should not point to a CAS URL but to
the image itself.
nmattia added a commit that referenced this pull request Nov 23, 2024
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.

3 participants