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

Validate implicit task names #4399

Merged

Conversation

oliver-sanders
Copy link
Member

@oliver-sanders oliver-sanders commented Sep 9, 2021

Built on #4343

Fixes small bug spotted whilst reviewing #4343 where it is possible to define root as an implicit task.

Also reserves _cylc.* in xtriggers and task names (used by internal xtriggers).

Testing checklist:

  • Implicit task names in graph section (only) caught by validate/play.
  • Implicit task names in runtime section (only) caught by validate/play.
  • Retry xtriggers still work ok.
  • Auto generated docs for the TaskNameValidator make sense.

Requirements check-list

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.py and
    conda-environment.yml.
  • Appropriate tests are included (unit and/or functional).
  • Appropriate change log entry included.
  • No documentation update required (auto documented from TaskNameValidator object)

@oliver-sanders oliver-sanders self-assigned this Sep 9, 2021
@oliver-sanders oliver-sanders force-pushed the validate-implicit-task-names branch from 77395c1 to cc74aad Compare September 9, 2021 10:44
@oliver-sanders oliver-sanders added this to the cylc-8.0.0 milestone Sep 9, 2021
@oliver-sanders oliver-sanders force-pushed the validate-implicit-task-names branch from cc74aad to 51d7641 Compare September 14, 2021 10:29
@oliver-sanders oliver-sanders added the bug Something is wrong :( label Sep 14, 2021
@oliver-sanders oliver-sanders force-pushed the validate-implicit-task-names branch from 51d7641 to 1428676 Compare September 17, 2021 11:05
@hjoliver
Copy link
Member

@oliver-sanders - note I pushed a trivial test fix to your branch, to see if all tests will pass here now.

@hjoliver
Copy link
Member

hjoliver commented Sep 22, 2021

... they do 🎉 ... any reason this is still a Draft? Change log entry?

@oliver-sanders
Copy link
Member Author

oliver-sanders commented Sep 22, 2021

Waiting for me to get around to rebasing, branch got in a mess.

@oliver-sanders oliver-sanders marked this pull request as ready for review September 22, 2021 10:38
@oliver-sanders oliver-sanders force-pushed the validate-implicit-task-names branch 2 times, most recently from 8bb1269 to 0a78b40 Compare September 22, 2021 10:41
@oliver-sanders oliver-sanders force-pushed the validate-implicit-task-names branch from 0a78b40 to fc31081 Compare October 6, 2021 09:20
@oliver-sanders oliver-sanders requested a review from wxtim October 6, 2021 09:21
@wxtim
Copy link
Member

wxtim commented Oct 6, 2021

Should this:

[scheduler]
    allow implicit tasks = true

[scheduling]
    initial cycle point = 1066

    [[dependencies]]
        P1Y = root

[runtime]
    [[foo]]

validate?

I don't think it should, but it does for me.

@oliver-sanders oliver-sanders force-pushed the validate-implicit-task-names branch from fc31081 to 9787622 Compare October 6, 2021 10:16
@oliver-sanders
Copy link
Member Author

Good catch, it turns out that the graph parser was expanding root to a list of tasks, but only if there were tasks defined in the runtime section.

Fixed and extended the test to cover this.

@oliver-sanders oliver-sanders force-pushed the validate-implicit-task-names branch from 9787622 to ce60824 Compare October 8, 2021 11:43
@oliver-sanders oliver-sanders modified the milestones: cylc-8.0rc1, cylc-8.0b3 Oct 8, 2021
@oliver-sanders oliver-sanders merged commit 380a3d2 into cylc:master Oct 8, 2021
@oliver-sanders oliver-sanders deleted the validate-implicit-task-names branch October 8, 2021 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants