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

Update pathogen-repo-ci to "generic"/"smart" workflow #89

Closed
Tracked by #84
genehack opened this issue May 30, 2024 · 5 comments
Closed
Tracked by #84

Update pathogen-repo-ci to "generic"/"smart" workflow #89

genehack opened this issue May 30, 2024 · 5 comments
Assignees
Labels
enhancement New feature or request

Comments

@genehack
Copy link
Contributor

genehack commented May 30, 2024

Context

Per recent discussion, as well as various legacy conversations here, here, and here, we want to update the pathogen-repo-ci GitHub Action to a "strict" version that can be used across all of our "modern" pathogen repos with zero configuration on the consuming repo side.

Description

Update the pathogen-repo-ci workflow so that:

  1. If the top-level nextstrain-pathogen.yaml file is present
  2. For each build-name (i.e., each of ingest, phylogenetic, and nextclade):
    a. If the repo contains a <build-name>/Snakefile file and a <build-name>/build-configs/ci/config.yaml file
    b. Run nextstrain build <build-name> --configfile build-configs/ci/config.yaml

The consuming repo will be responsible for providing an appropriate config file that either sets up appropriate example data or results in a subset of a full workflow being run, so as to validate that the repo workflows have not been broken by a change.

Examples

This workflow, plus the appropriate build-config files, should be sufficient to enable CI for a pathogen repo:

name: CI
on:
  - push
  - workflow_dispatch
jobs:
  ci:
    name: CI
    uses: nextstrain/.github/workflows/pathogen-repo-ci.yaml
@joverlee521
Copy link
Contributor

joverlee521 commented May 30, 2024

100% support this direction.

One additional check I would add to the pathogen-repo-ci is the presence of the top level nextstrain-pathogen.yaml file within the pathogen repo. This ensures nextstrain build is able to access of files between subdirectories, e.g. both phylogenetic and nextclade might need to access files from the top level shared directory. (nextstrain-cli v8.2.0 release notes has more details.)

@tsibley
Copy link
Member

tsibley commented May 31, 2024

+1 from me.

For the calling workflow in each repo, I'd extend the default set of triggers, e.g. something like

on:
  push:
    branches:
      - main # or "master", as required
  pull_request:
  workflow_dispatch:

  # Routinely check that we continue to work in the face of external changes.
  schedule:
    # Every day at 17:42 UTC / 9:42 Seattle (winter) / 10:42 Seattle (summer)
    - cron: "42 17 * * *"

c.f. https://github.com/nextstrain/.github/blob/5ab8d7aa3c97ca3257dfa9fc201f1463034f3e6d/workflow-templates/pathogen-repo-ci.yaml as well

@genehack
Copy link
Contributor Author

Let me tug on this thread a little...

push:
    branches:
      - main # or "master", as required
  pull_request:
  workflow_dispatch:

Generally, for CI, I want it to run when anything changes — like, I want to be able to push a branch and trigger it without opening a PR (because I'm still developing and just want the sanity check that I haven't broken anything so far).

(Aside: At one point I had both push (no branch condition, just push) and pull_request in an action, and I noticed the action was running twice whenever I opened a PR, which is obviously wasteful.)

The workflow_dispatch trigger is obviously useful, but to me it feels like a CI action can leave off the pull_request trigger in favor of an unconditional push trigger.

@genehack
Copy link
Contributor Author

One additional check I would add to the pathogen-repo-ci is the presence of the top level nextstrain-pathogen.yaml file

Added to the sketch above.

@tsibley
Copy link
Member

tsibley commented May 31, 2024

I understand where you're coming from, but I disagree. From my rationale 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.

genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
This should be handled within the build-config of the consuming repo
if it is required.
genehack added a commit that referenced this issue May 31, 2024
@genehack genehack mentioned this issue May 31, 2024
1 task
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
genehack added a commit that referenced this issue May 31, 2024
…89]

Has the side benefit of allowing the `run-nexstrain-build` action to
also be invoked from the sidecar instead of pinned to `master` or some
other version.
genehack added a commit that referenced this issue May 31, 2024
…89]

Has the side benefit of allowing the `run-nexstrain-build` action to
also be invoked from the sidecar instead of pinned to `master` or some
other version.
genehack added a commit that referenced this issue May 31, 2024
…89]

Has the side benefit of allowing the `run-nexstrain-build` action to
also be invoked from the sidecar instead of pinned to `master` or some
other version.
genehack added a commit that referenced this issue May 31, 2024
Also update nextstrain runtime setup step to use "modern" sidecar
version, which has the side effect/benefit of allowing the
`run-nexstrain-build` action to also be invoked from the sidecar
instead of pinned to `master` or some other version.
genehack added a commit that referenced this issue Jun 1, 2024
* run-nextstrain-build-step -> run-nextstrain-build
* inputs.step -> inputs.directory
genehack added a commit that referenced this issue Jun 4, 2024
Also update nextstrain runtime setup step to use "modern" sidecar
version, which has the side effect/benefit of allowing the
`run-nexstrain-build` action to also be invoked from the sidecar
instead of pinned to `master` or some other version.
genehack added a commit that referenced this issue Jun 4, 2024
* run-nextstrain-build-step -> run-nextstrain-build
* inputs.step -> inputs.directory
genehack added a commit that referenced this issue Jun 4, 2024
…per PR feedback

* ignore/don't warn when no files found
* include `auspcice` directory in upload set
* sort lines
genehack added a commit that referenced this issue Jun 4, 2024
…89]

* Pass `runtime` as an input, rather than accessing `matrix.runtime`
  directly
* Update action description to reflect dependency on having Nextstrain
  CLI already set up via other action; add warning about the purpose
  of this action
genehack added a commit that referenced this issue Jun 7, 2024
* Update `artifact-name` description in `pathogen-repo-ci.yaml` to
  make it clear build directory will be appended to provided value,
  and to update default value to make clear these are CI results
* Pass `artifact-name` down into `run-nextstrain-ci-build` steps ni
  `pathogen-repo-ci` workflow
* Add `artifact-name` input to `run-nextstrain-ci-build` action and
  use it in `upload-artifact` action step
@genehack genehack self-assigned this Jun 10, 2024
tsibley added a commit that referenced this issue Jun 12, 2024
No longer configurable as of "Refactor `pathogen-repo-ci` to be smarter
[#89]" (d3730bc).
tsibley added a commit that referenced this issue Jun 14, 2024
No longer configurable as of "Refactor `pathogen-repo-ci` to be smarter
[#89]" (d3730bc).
tsibley added a commit that referenced this issue Jun 14, 2024
No longer configurable as of "Refactor `pathogen-repo-ci` to be smarter
[#89]" (d3730bc).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants