test: Refactor IC-OS image config in system_tests.bzl for more clarity #8436
Conversation
- Instead of the `uses_*` pattern, pass `hostos` and `setupos` args to `system_test` similarly to `guestos`. - Factor out repeating parts of the config in `system_tests.bzl` into functions - Behavior change: `recovery-dev` now builds the initial update image corresponding to the base image (both from `//ic-os/guestos/envs/recovery-dev`), whereas previously the two weren't consistent. The test still passes and I got green light from @andrew who originally wrote the tests.
…ests-icos-config # Conflicts: # rs/tests/nested/BUILD.bazel # rs/tests/testnets/BUILD.bazel
frankdavid
added
the
CI_ALL_BAZEL_TARGETS
| def _configure_icos(env, env_var_files, icos_images, runtime_deps, guestos, guestos_update, setupos, hostos_update): | ||
| """Configure all IC-OS images based on the provided parameters.""" | ||
|
|
||
| is_mainnet_dev = guestos == "mainnet_latest_dev" |
There was a problem hiding this comment.
If guestos == "mainnet_latest_dev", then we use dev mainnet images, which effects both setupos_mainnet and hostos_update_mainnet. This is intentional, as we want to consistently use dev mainnet images across all the OSes. But it becomes confusing if you look at the guestos_upgrade_from_latest_release_to_current target declaration:
guestos = "mainnet_latest_dev", # necessary for configuring NNS public key
...
setupos = "mainnet_latest",
But the guestos mainnet_latest_dev causes the setupos used to actually be "mainnet_latest_dev"
I think it would be cleaner to add a uses_mainnet_dev flag, and then get rid of the guestos mainnet_latest_dev option. Then, the use of mainnet dev images is something targets directly enable or not.
I know you didn't introduce this behavior in this PR, but this seems like a nice chance to improve it 🙂
There was a problem hiding this comment.
Also, I see that guestos_update_mainnet doesn't use is_mainnet_dev. This matches the behavior before your change.
I'm not sure why we have this behavior though.
We appear to be uploading the dev mainnet images:
Line 281 in 8f51fe4
So it shouldn't be a problem to use dev mainnet update images.
If you can't think of a reason why we shouldn't be using dev guestos update mainnet images, I think we can also update guestos_update_mainnet to follow a uses_mainnet_dev flag.
Also, it would be good to add a check that uses_mainnet_dev = True is only used when at least one *_mainnet image is specified
|
Great refactor! |
4 participants
uses_*pattern, passhostosandsetuposargs tosystem_testsimilarly toguestos.system_tests.bzlinto functionsrecovery-devnow builds the initial update image corresponding to the base image (both from//ic-os/guestos/envs/recovery-dev), whereas previously the two weren't consistent. The test still passes and I got green light from @andrewbattat who originally wrote the tests.