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: Build Auspice client #599

Closed
wants to merge 2 commits into from
Closed

Conversation

victorlin
Copy link
Member

Description of proposed changes

It can be useful to test that a newer Auspice version builds properly in the context of this project.

Related issue(s)

Testing scenario brought up by @jameshadfield in nextstrain/auspice#1520 (comment).

Testing

  • Checks pass

victorlin and others added 2 commits October 4, 2022 16:02
Same reasoning as @tsibley in nextstrain/cli@fab709a:

Running on push _and_ PRs is often redundant.  For PRs, we really care
about the putative merge of the PR branch, and that's what "on:
pull_request" tests.  We typically do not need push-level CI results for
PRs.  On the other hand, CI results for every push to master are nice to
have both as a safety backstop and for the linear chain of CI history it
produces (e.g. to debug the impact of external changes on our CI).

The primary downside I see is that you can no longer push without
opening a PR just to see what CI says, but I think that's an acceptable
tradeoff, especially now that draft PRs are a thing.  To mitigate this
downside, "on: workflow_dispatch" allows CI to be manually dispatched
for a specific branch/tag/commit if you _really_ don't want to open even
a draft PR.

Trimming unnecessary CI jobs reduces the time to completion for CI runs
(good for the dev loop) and reduces organization-level job queuing,
which can negatively impact the workflows of other repos.

Co-authored-by: Thomas Sibley <[email protected]>
It can be useful to test that a newer Auspice version builds properly in
the context of this project.
@victorlin victorlin requested a review from a team October 4, 2022 23:04
@victorlin victorlin self-assigned this Oct 4, 2022
@nextstrain-bot nextstrain-bot temporarily deployed to nextstrain-s-victorlin--5z2czf October 4, 2022 23:05 Inactive
@jameshadfield
Copy link
Member

This should be covered by our normal CI (npm run build will build the auspice client), right?

The scenario I brought up is where we want to try to build a specific (new / development) version of auspice within the nextstrain.org repo. This requires something along the lines of npm install nextstrain/auspice#commit (you can do it locally with npm link but I find it more robust to use a github commit). I think the way to achieve this is for an auspice action to make a testing PR within nextstrain.org (i.e. nextstrain/auspice#1565). Perhaps I'm missing something here.

@victorlin
Copy link
Member Author

This should be covered by our normal CI (npm run build will build the auspice client), right?

@jameshadfield oh you're right! This PR isn't needed then. Yes, nextstrain/auspice#1565 should be sufficient.

@victorlin victorlin closed this Oct 5, 2022
@victorlin victorlin deleted the victorlin/ci-build-auspice-client branch October 5, 2022 17:08
@tsibley
Copy link
Member

tsibley commented Oct 5, 2022

I'd still +1 this though: b057b77

@victorlin
Copy link
Member Author

@tsibley oh I forgot about that! Done with #601

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.

4 participants