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] Clarify messaging around which runners are updating #87

Closed
1 task
tsibley opened this issue Jun 17, 2020 · 0 comments
Closed
1 task

[update] Clarify messaging around which runners are updating #87

tsibley opened this issue Jun 17, 2020 · 0 comments
Assignees
Labels
enhancement New feature or request

Comments

@tsibley
Copy link
Member

tsibley commented Jun 17, 2020

nextstrain update predates runners other than Docker and its messaging assumes that Docker is the runner in use. This isn't always the case anymore, and the output of update can be confusing as it's a mix of messages the CLI prints directly as well as output from docker image pull.

The update command could either operate on all supported runners (like nextstrain check-setup) or only operate on the default runner. In either case, runners other than Docker would want to print that they don't currently support updating. (In the future they might.)

  • Remove the || true and associated todo comment in our CI workflow when this issue is fixed:
    # XXX TODO: Stop ignoring errors once `update` is improved. See
    # <https://github.com/nextstrain/cli/issues/87>.
    nextstrain update || true
@tsibley tsibley added the enhancement New feature or request label Jun 17, 2020
@tsibley tsibley self-assigned this Sep 8, 2022
tsibley added a commit that referenced this issue Sep 13, 2022
With no arguments, the default runtime is updated.  The name of another
runtime to update instead may be provided as an argument.

In practice this isn't much of a behaviour change because only one
runtime currently supports updating (Docker); the others (native, AWS
Batch) just pass.  Existing users are unlikely to notice the change
unless they use multiple runtimes and Docker is not their default.  In
that case, `update` may stop updating Docker for them when it would have
done so previously.  The output of `update` should make this clear
though.

This change also improves the help, output, and other messaging of
`update` to not assume that Docker is the only runner in use.  (It
predated runners other than Docker in this codebase.)  As part of this,
the signature of RunnerModule.test_setup() now allows a "not supported"
(None) return value, in addition to the existing boolean for
success/failure.

Resolves <#87>.
tsibley added a commit that referenced this issue Sep 14, 2022
With no arguments, the default runtime is updated.  The name of another
runtime to update instead may be provided as an argument.

In practice this isn't much of a behaviour change because only one
runtime currently supports updating (Docker); the others (native, AWS
Batch) just pass.  Existing users are unlikely to notice the change
unless they use multiple runtimes and Docker is not their default.  In
that case, `update` may stop updating Docker for them when it would have
done so previously.  The output of `update` should make this clear
though.

This change also improves the help, output, and other messaging of
`update` to not assume that Docker is the only runner in use.  (It
predated runners other than Docker in this codebase.)  As part of this,
the signature of RunnerModule.test_setup() now allows a "not supported"
(None) return value, in addition to the existing boolean for
success/failure.

Resolves <#87>.
@victorlin victorlin moved this from New to In Review in Nextstrain planning (archived) Sep 20, 2022
tsibley added a commit that referenced this issue Sep 23, 2022
With no arguments, the default runtime is updated.  The name of another
runtime to update instead may be provided as an argument.

In practice this isn't much of a behaviour change because only one
runtime currently supports updating (Docker); the others (native, AWS
Batch) just pass.  Existing users are unlikely to notice the change
unless they use multiple runtimes and Docker is not their default.  In
that case, `update` may stop updating Docker for them when it would have
done so previously.  The output of `update` should make this clear
though.

This change also improves the help, output, and other messaging of
`update` to not assume that Docker is the only runner in use.  (It
predated runners other than Docker in this codebase.)  As part of this,
the signature of RunnerModule.test_setup() now allows a "not supported"
(None) return value, in addition to the existing boolean for
success/failure.

Resolves <#87>.
tsibley added a commit that referenced this issue Sep 26, 2022
With no arguments, the default runtime is updated.  The name of another
runtime to update instead may be provided as an argument.

In practice this isn't much of a behaviour change because only one
runtime currently supports updating (Docker); the others (native, AWS
Batch) just pass.  Existing users are unlikely to notice the change
unless they use multiple runtimes and Docker is not their default.  In
that case, `update` may stop updating Docker for them when it would have
done so previously.  The output of `update` should make this clear
though.

This change also improves the help, output, and other messaging of
`update` to not assume that Docker is the only runner in use.  (It
predated runners other than Docker in this codebase.)  As part of this,
the signature of RunnerModule.test_setup() now allows a "not supported"
(None) return value, in addition to the existing boolean for
success/failure.

Resolves <#87>.
tsibley added a commit that referenced this issue Sep 26, 2022
With no arguments, the default runtime is updated.  The name of another
runtime to update instead may be provided as an argument.

In practice this isn't much of a behaviour change because only one
runtime currently supports updating (Docker); the others (native, AWS
Batch) just pass.  Existing users are unlikely to notice the change
unless they use multiple runtimes and Docker is not their default.  In
that case, `update` may stop updating Docker for them when it would have
done so previously.  The output of `update` should make this clear
though.

This change also improves the help, output, and other messaging of
`update` to not assume that Docker is the only runner in use.  (It
predated runners other than Docker in this codebase.)  As part of this,
the signature of RunnerModule.test_setup() now allows a "not supported"
(None) return value, in addition to the existing boolean for
success/failure.

Resolves <#87>.
@tsibley tsibley closed this as completed Oct 6, 2022
Repository owner moved this from In Review to Done in Nextstrain planning (archived) Oct 6, 2022
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
No open projects
Development

No branches or pull requests

1 participant