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

doc: convert config guide to reST, document name restrictions for builds: #896

Merged
merged 2 commits into from
Mar 29, 2022

Conversation

victorlin
Copy link
Member

@victorlin victorlin commented Mar 22, 2022

Description of proposed changes

This PR adds a warning message to the builds section:

image

This is documented internally with workflow config schema and various wildcard constraints.

To add the warning block, I first had to convert to reStructuredText.

Related issue(s)

Testing

N/A

Release checklist

N/A

pandoc -f markdown -t rst --wrap=none docs/src/reference/configuration.md -o docs/src/reference/configuration.rst
@victorlin victorlin force-pushed the victorlin/document-name-restrictions branch 3 times, most recently from 4ca2512 to d0459c7 Compare March 23, 2022 18:12
@victorlin victorlin requested a review from a team March 25, 2022 22:06
@jameshadfield
Copy link
Member

Thanks @victorlin! Looks good. Perhaps explicitly call out the fact that they cannot have a full stop?

Also, the schema should be updated to avoid the build name ending with _measurements or _root-sequence, both of which are reserved for sidecar files. But this can be another PR / issue.

John & I talked about re-organising this file, and I'll try to do this in a separate PR on top of the tutorial branch.

@victorlin victorlin force-pushed the victorlin/document-name-restrictions branch from d0459c7 to f8c19cf Compare March 28, 2022 21:34

.. warning::

Build names currently only allow alpha characters, underscores, and hyphens (``A-Z``, ``a-z``, ``_``, ``-``), but must not contain ``tip-frequencies`` as it is a special string used for Nextstrain builds.
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 also noticed that just tip-frequencies passes the regex, is this fine? Or should it be matching for _tip-frequencies?

Copy link
Member

Choose a reason for hiding this comment

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

We should err on the side of caution here and make sure the build name doesn't end with any of the 5 suffixes we filter out in the nextstrain server:

  // All JSON files which aren't a sidecar file with a known suffix are assumed to
  // be v2+ JSONs (aka "unified" JSONs)
  const sidecarSuffixes = ["meta", "tree", "root-sequence", "seq", "tip-frequencies", "measurements"];
  const datasets = jsonFiles
    .filter((filename) => !sidecarSuffixes.some((suffix) => filename.endsWith(`_${suffix}.json`)))

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this to issue #900

@victorlin victorlin mentioned this pull request Mar 28, 2022
2 tasks
@victorlin victorlin merged commit 2399e77 into master Mar 29, 2022
@victorlin victorlin deleted the victorlin/document-name-restrictions branch March 29, 2022 16:55
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.

2 participants