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

proposal: universal id #115

Merged
merged 5 commits into from
Nov 2, 2020
Merged

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Oct 27, 2020

Adopt a universal identifier throughout all Cylc projects:

Ping @dwsutherland

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

Great, let's do it. 👍

Solves a bunch of problems, and doesn't create new ones that I can think of, except that we'll need a back-compat converter for Cylc 7 syntax to avoid breaking existing suites?

docs/proposal-universal-id.md Outdated Show resolved Hide resolved
docs/proposal-universal-id.md Outdated Show resolved Hide resolved
@oliver-sanders
Copy link
Member Author

Added a new "back-compat" section with an overview of the effected commands and an outline for supporting the Cylc7 syntax until Cylc9.

[~user[/]]flow//cycle/task/job

# expanded form with selectors (for quick querying of objects)
[~user[/]]flow[:state]//cycle[:state]/task[:state]/job[:state]
Copy link
Member

Choose a reason for hiding this comment

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

[~user[/]]flow

What if flow has / in it (path hierarchy structure) .. How do you know if user has been included? is it the ~?

cycle[:state]

I thought we ruled out : colon? Because some ISO datetime uses it

Copy link
Member Author

@oliver-sanders oliver-sanders Nov 1, 2020

Choose a reason for hiding this comment

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

is it the ~?

Yep.

Should make it nice and explicit when working with other users workflows, ~ isn't a valid character in a workflow registration.

I thought we ruled out : colon? Because some ISO datetime uses it

I think we should be good to use the colon because Cylc doesn't use colons for its cycle point format, and can't because colons are not safe in file paths.

> **Note:** All syntax implemented here represents functionality currently
supported in the GraphQL schema.

> **Note:** Currently in GraphQL `state` is a separate argument, there is no
Copy link
Member

@dwsutherland dwsutherland Oct 30, 2020

Choose a reason for hiding this comment

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

Currently our GraphQL API can do both, this was intentional.. I can't recall the exact reason, but I think it was so you could do blanket exclusions and/or inclusions of states while also picking specific named tasks (?)

Copy link
Contributor

@dpmatthews dpmatthews left a comment

Choose a reason for hiding this comment

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

A few comments but happy to approve


cylc trigger OPTIONS ID
cylc trigger my_workflow//123/task
# currently: cylc stop my_workflow 123/task (or task.123)
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: should be cylc trigger not stop

cylc trigger my_workflow//123/task
# currently: cylc stop my_workflow 123/task (or task.123)
cylc trigger my_workflow//123/*:failed
# currently: cylc stop my_workflow 123/*:failed
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Comment on lines +196 to +197
* Enables multi-user control from the CLI via the UIS without Cylc Flow having
to know anything about authorisation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this (but not important at this stage)


## Back Compat

We should be able to support the old legacy interface for the time being.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd be happy to drop the legacy interface for most commands and only worry about those we think may have been used within scripts/workflows (e.g. broadcast).

Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

Looks good, will be nice to have the internal and external ID unified.

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.

4 participants