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

Fix more workflow-state docs. #35

Open
wants to merge 1 commit into
base: workflow-state-doc
Choose a base branch
from

Conversation

hjoliver
Copy link

Side PR to cylc#6477

Comment on lines -23 to +27
The ID argument can target a workflow, or a cycle point, or a specific
task, with an optional selector on cycle or task to match task status,
output trigger (if not a status, or with --trigger) or output message
(with --message). All matching results will be printed.
The ID argument can target a workflow, or a cycle point, or a specific task,
with an optional selector on cycle or task to match final task statuses,
output trigger (with --triggers), or output message (with --messages).
You cannot poll for transient states such as "submitted" and "running".
Poll for the corresponding output triggers instead ("submitted", "started").
Copy link
Author

Choose a reason for hiding this comment

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

Corrections:

  • no longer accepts transient statuses, only final statuses
  • no longer falls back to trigger if not a known status
  • --message was wrong (it's --messages)


In "cycle/task:selector" the selector will match task statuses, unless:
- if it is not a known status, it will match task output triggers
Copy link
Author

Choose a reason for hiding this comment

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

  • no longer true

Comment on lines -67 to -68
- To avoid missing transient states ("submitted", "running") poll for the
corresponding output trigger instead ("submitted", "started").
Copy link
Author

Choose a reason for hiding this comment

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

This doesn't belong under warnings anymore as we now refuse to poll for transient statuses. I've added the advice on final statuses and triggers to the main text above.

@@ -370,7 +369,7 @@ def main(parser: COP, options: 'Values', *ids: str) -> None:
[
options.depr_task,
options.depr_status,
options.depr_msg, # --message and --trigger
Copy link
Author

Choose a reason for hiding this comment

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

Comment was wrong and isn't really necessary anyway.
There was never any --trigger option; it is --output and --message (singular) that are deprecated.

Comment on lines +45 to +46
- with --messages, it will only match task output messages. DEPRECATED
use triggers instead - they match manually and naturally set outputs.
Copy link
Owner

Choose a reason for hiding this comment

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

Not deprecated though?

Suggested change
- with --messages, it will only match task output messages. DEPRECATED
use triggers instead - they match manually and naturally set outputs.
- with --messages, it will only match task output messages. It is recommended
to use triggers instead - they match manually and naturally set outputs.

Copy link
Author

Choose a reason for hiding this comment

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

It is deprecated, isn't it? We support querying output messages for back-compatibility, but there's no good reason to choose messages over triggers now, so we should eventually drop the option.

Copy link
Owner

@MetRonnie MetRonnie Nov 14, 2024

Choose a reason for hiding this comment

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

There's no deprecation warning with the --messages option

Copy link
Author

@hjoliver hjoliver Nov 14, 2024

Choose a reason for hiding this comment

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

Hmm. Maybe it should have one. Not sure if I imagined the deprecation, just thought it should be deprecated, or got the idea from Oliver via his earlier tidy-up PR - ping @oliver-sanders ?

Choose a reason for hiding this comment

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

I think the deprecated applies to querying by task state, not message.

Querying by task state can be flaky, it would be reasonable to deprecate this.

Querying by task message isn't much different to querying by task trigger. It might be useful if the message isn't registered as an output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants