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

dev: Routinely check that the latest* package is initially installable… #42

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

tsibley
Copy link
Member

@tsibley tsibley commented Sep 15, 2023

…to catch issues like the one in monkeypox CI¹ earlier. Since
successful installation relies on external resources out of our control,
we want to regularly test it to ensure we know when an external change
breaks it.

* As it stands currently, this isn't strictly the latest package, just
that there's some package version that's installable. To ensure
the former, we'd have to query the latest version (e.g. similar to
what devel/download-latest does) and pass it into setup-nextstrain-cli
as an input, which would then pass it to nextstrain in
NEXTSTRAIN_CONDA_BASE_PACKAGE.

Alternatively, we're likely to update nextstrain setup conda anyway
to query and install the latest version itself explicitly, just like
nextstrain update conda does, and so this workflow can simply wait
for that change to happen.

Resolves #41.

¹ e.g. nextstrain/mpox#177

Checklist

  • Update commit message and code commentary re: "latest"
  • Checks pass

@tsibley
Copy link
Member Author

tsibley commented Sep 15, 2023

Test run: https://github.com/nextstrain/conda-base/actions/runs/6202996684

However, this as-is right now tests only that there exists some package version that's initially installable. It doesn't guarantee that package version is the latest. We could do that two ways:

  1. Query the latest version (e.g. similar to what devel/download-latest does) and pass it into setup-nextstrain-cli as an input, which would then pass it to nextstrain in NEXTSTRAIN_CONDA_BASE_PACKAGE.
  2. Update nextstrain setup conda to query the latest version and explicitly install that, just like nextstrain update conda does (c.f.).

I think (2) makes the most sense. In the meantime, testing that any version is installable is still useful, and when we update Nextstrain CLI for (2) this workflow will start always testing the latest version.

@tsibley tsibley marked this pull request as draft September 15, 2023 21:41
tsibley added a commit to nextstrain/cli that referenced this pull request Sep 15, 2023
This our intent and expectation, and it's good to be explicit about it.
It may surface more installation issues, such as the one we observed in
monkeypox CI¹ with the latest version not being installable, but
obscuring those or surfacing them later on is _probably_ worse than
addressing them head on earlier.  This change will mean that any macOS
10.14 users, if any, would have to use

    NEXTSTRAIN_CONDA_BASE_PACKAGE="nextstrain-base ==20230615T171309Z"

since newer versions aren't installable for them.²

This behaviour also parallels update's behaviour since "runner.conda:
Explicitly specify a nextstrain-base version when updating" (d6e4f2b).
It was an oversight (on my part) to not use the same behaviour during
setup, but at the time I was focused on fixing an update bug.

Related-to: <nextstrain/conda-base#41>
Related-to: <nextstrain/conda-base#42>

¹ <nextstrain/mpox#177>
² <nextstrain/conda-base#38>
@tsibley
Copy link
Member Author

tsibley commented Sep 15, 2023

nextstrain/cli#312

@tsibley tsibley force-pushed the trs/test-installation-regularly branch from e2091c6 to c74a4a3 Compare September 18, 2023 16:48
@tsibley tsibley changed the title dev: Routinely check that the latest package is initially installable… dev: Routinely check that the latest* package is initially installable… Sep 18, 2023
@tsibley tsibley marked this pull request as ready for review September 18, 2023 17:44
@tsibley tsibley requested a review from a team September 18, 2023 17:44
Copy link
Member

@victorlin victorlin left a comment

Choose a reason for hiding this comment

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

Commit message can be updated now that nextstrain/cli#312 is merged.

…to catch issues like the one in monkeypox CI¹ earlier.  Since
successful installation relies on external resources out of our control,
we want to regularly test it to ensure we know when an external change
breaks it.

Resolves <#41>.

¹ e.g. <nextstrain/mpox#177>
@tsibley tsibley force-pushed the trs/test-installation-regularly branch from c74a4a3 to a64df84 Compare October 9, 2023 23:14
@tsibley
Copy link
Member Author

tsibley commented Oct 9, 2023

Updated. Test run worked. Merging.

@tsibley tsibley merged commit 19e3ef4 into main Oct 9, 2023
24 of 25 checks passed
@tsibley tsibley deleted the trs/test-installation-regularly branch October 9, 2023 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Routinely test that the latest package version is initially installable
2 participants