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.

Sign up for GitHub

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(pathogen-repo-ci): allow configuration of workflow root #57

Merged
merged 2 commits into from
Sep 25, 2023
Merged

feat(pathogen-repo-ci): allow configuration of workflow root #57

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2aa21a6
feat(pathogen-repo-ci): allow configuration of workflow root
corneliusroemer Sep 25, 2023
dec0880
Update .github/workflows/pathogen-repo-ci.yaml
corneliusroemer Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions .github/workflows/pathogen-repo-ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@ on:
- conda
required: false

workflow-root:
Copy link
Member

@victorlin victorlin Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

"Workflow" is an overloaded term in this context (GitHub workflow vs. pathogen workflow). What about a simpler name like path, root, or directory? That would look like:

jobs:
  pathogen-ci:
    uses: nextstrain/.github/.github/workflows/pathogen-repo-ci.yaml@master
    with:
      path: phylogenetic/

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'm not sure the simpler path is as clear. Yes there's ambiguity in workflow-root - but path is even more ambiguous.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, also fine for this to be non-blocking. It works, I don't have any better suggestions, and this is something we can change easily if it's only used internally.

Copy link
Member

Choose a reason for hiding this comment

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

build-dir, i.e. "the path for nextstrain build"

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be happy with build-dir or build-root

description: >-
Path to the root of the Snakemake workflow filder to run CI on.
corneliusroemer marked this conversation as resolved.
Show resolved Hide resolved
Defaults to the root of the workflow repo.
type: string
default: ${{ github.workspace }}
required: false

artifact-name:
description: >-
Name to use for build results artifact uploaded at the end of the
Expand Down Expand Up @@ -228,6 +236,7 @@ jobs:
python-version: "3.7"

- name: Copy example data
working-directory: ${{ inputs.workflow-root }}
run: |
if [[ -d example_data ]]; then
mkdir -p data/
Expand All @@ -237,14 +246,17 @@ jobs:
fi

- run: nextstrain build . ${{ inputs.build-args }}
working-directory: ${{ inputs.workflow-root }}

- if: always()
uses: actions/upload-artifact@v3
env:
workdir: ${{ inputs.workflow-root }}
with:
name: ${{ inputs.artifact-name }}-${{ matrix.runtime }}
path: |
auspice/
results/
benchmarks/
logs/
.snakemake/log/
$workdir/auspice/
$workdir/results/
$workdir/benchmarks/
$workdir/logs/
$workdir/.snakemake/log/
Comment on lines +258 to +262
Copy link
Member

Choose a reason for hiding this comment

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

This currently only accepts paths without a trailing slash. I think it'd be good to accept both with and without, because even if we document support for only one in the input description, the error that happens here might not be very informative and only happens after everything else is done running.

I don't think there's an easy way to do something like Python's os.path.join inline here, so the solution might be to normalize as a first step before using the value.

Copy link
Member Author

Choose a reason for hiding this comment

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

This sounds like a non-blocking feature request to me :)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it works, so this can be non-blocking.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a non-blocking feature request to me :)

This sounds like a flippant response to a reasonable request for robustness.

The trailing slash is not even a documented requirement in the input description. But even if it was, requiring a slash is an easily-avoided tripping hazard that we don't need to subject ourselves to.

Comment on lines 251 to +262
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work. The interpolation does not work like that. If you look at the "successful" test, you'll see it didn't find any files and uploaded no artifacts.

Loading