Cylc config add `--platform-names` #4576

wxtim · 2022-01-12T11:27:42Z

These changes close #4557

Asking for a review before I add changelog or doc.

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.cfg and conda-environment.yml.
Appropriate tests are included (unit and/or functional).
Appropriate change log entry included.
(master branch) I have opened a documentation PR at cylc/cylc-doc/pull/XXXX.

cylc/flow/scripts/config.py

tests/unit/cfgspec/test_globalcfg.py

tests/unit/scripts/test_config.py

cylc/flow/scripts/config.py

cylc/flow/cfgspec/globalcfg.py

oliver-sanders

Looks fine, need to complete checkboxes in desc.

oliver-sanders · 2022-01-20T10:36:15Z

tests/f/cylc-config/09-platforms.t failing

wxtim · 2022-01-20T16:11:25Z

tests/f/cylc-config/09-platforms.t failing

Should have that fixed.

oliver-sanders · 2022-01-21T16:44:22Z

Tests are passing, looks good, just needs rebase squash'n.

…ed tests/cylc-get-config to tests/cylc-config

MetRonnie

Small thing: with no [platform groups] in my global config,

$ cylc config --platform-meta
...
        submission polling intervals = PT1S
    [[_remote_background_shared_ssh]]
        hosts = els055
        communication method = ssh
        install target = localhost
ItemNotFoundError: You have not set "platform groups" in this config.

$ echo $?
1

MetRonnie · 2022-01-24T19:06:09Z

cylc/flow/scripts/config.py

+    options_are_valid(options, workflow_id)
+


Shouldn't this be at the top of the function?

Do you mean that the get_option_parser should always be directly above the main functions whose wrapper uses it?

I thought that it ought to go at the top, because it acts as a form of internal documentation. main() is generally written at the bottom, and that leaves these convenience functions in the middle.

The checking of options' validity should be at the beginning of main(), otherwise it's missed out. Only problem is that workflow_id is not defined until down here too

Sorry, I missed your meaning: In general I'd agree, but you've spotted why I can't do that.

So what I'm saying is there needs to be a change to allow the validation to come first, otherwise it's not doing its job

I've pushed a change to fix this.

cylc/flow/scripts/config.py

tests/unit/scripts/test_config.py

hjoliver

ItemNotFoundError: You have not set "platform groups" in this config.

Similarly, if no platforms are defined:

$ cylc config --platform-meta      
ItemNotFoundError: You have not set "platforms" in this config.

It should just print nothing, and exit 0.

cylc/flow/cfgspec/globalcfg.py

cylc/flow/scripts/config.py

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

…/cylc into cylc_config_-_add_--platform-names * 'cylc_config_-_add_--platform-names' of github.com:wxtim/cylc: Apply suggestions from code review Update cylc/flow/scripts/config.py

CHANGES.md

cylc/flow/cfgspec/globalcfg.py

…/cylc into cylc_config_-_add_--platform-names * 'cylc_config_-_add_--platform-names' of github.com:wxtim/cylc: Update cylc/flow/scripts/config.py

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

wxtim · 2022-01-26T13:26:36Z

@MetRonnie - I think that I've resolved Hilary's suggestion, and responded to your outstanding comment.

…ed tests/cylc-get-config to tests/cylc-config

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

…/cylc into cylc_config_-_add_--platform-names * 'cylc_config_-_add_--platform-names' of github.com:wxtim/cylc: Apply suggestions from code review response to review Update cylc/flow/scripts/config.py refactor testing Apply suggestions from code review Update cylc/flow/scripts/config.py created cylc config --platform-names and --platform-meta options; Moved tests/cylc-get-config to tests/cylc-config

MetRonnie · 2022-01-26T16:07:34Z

Sorry for not being clear - I've added a comment: #4576 (comment)

oliver-sanders · 2022-01-26T17:30:48Z

I think you've merged something into this branch that wasn't supposed to be there, the diff is now much larger than expected.

MetRonnie · 2022-01-26T17:43:14Z

Might be a case of GitHub dot com UI not coping (locally checked out in VSCode only showing 22 files changed compared to master). But the latest commit edd8bf8 is certainly strange, and doesn't appear to be needed?

…g_-_add_--platform-names * 'master' of https://github.com/cylc/cylc: added cylc get-resources --tutorials (cylc#4580) GH Actions: allow debug of tutorial workflow tests in event of timeout

wxtim · 2022-01-26T19:38:35Z

@MetRonnie and @oliver-sanders - I've merged with master, and the extra diff is gone again.

hjoliver · 2022-01-26T23:47:26Z

(~~Some~~ One? broken test s)

hjoliver · 2022-01-27T00:31:42Z

@wxtim - please check my final commits on this branch in case you think further tweaks are warranted.

Just because "time is of the essence" for rc1 release, I've pushed up:

a func test fix
refactored option checking a bit (this involved removing a unit test that might need replacing in some form)

If CI passes I'll merge...

hjoliver

🎉

…names Cylc config add `--platform-names`

wxtim requested review from hjoliver and oliver-sanders January 12, 2022 11:27

wxtim self-assigned this Jan 12, 2022

wxtim added the small label Jan 12, 2022

wxtim added this to the cylc-8.0.0 milestone Jan 12, 2022

oliver-sanders reviewed Jan 12, 2022

View reviewed changes

wxtim force-pushed the cylc_config_-_add_--platform-names branch 2 times, most recently from 1f38a9e to 9af97bc Compare January 17, 2022 07:43

wxtim requested a review from oliver-sanders January 17, 2022 07:43

oliver-sanders reviewed Jan 17, 2022

View reviewed changes

cylc/flow/scripts/config.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Jan 17, 2022

View reviewed changes

cylc/flow/cfgspec/globalcfg.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Jan 17, 2022

View reviewed changes

wxtim mentioned this pull request Jan 19, 2022

documentation for cylc config --platform-* cylc/cylc-doc#379

Merged

wxtim requested a review from oliver-sanders January 19, 2022 10:17

oliver-sanders approved these changes Jan 21, 2022

View reviewed changes

oliver-sanders requested review from datamel and removed request for hjoliver January 21, 2022 16:45

created cylc config --platform-names and --platform-meta options; Mov…

ac32e7e

…ed tests/cylc-get-config to tests/cylc-config

wxtim force-pushed the cylc_config_-_add_--platform-names branch from 1ba95d5 to ac32e7e Compare January 23, 2022 10:38

oliver-sanders requested review from MetRonnie and removed request for datamel January 24, 2022 11:07

MetRonnie reviewed Jan 24, 2022

View reviewed changes

hjoliver requested changes Jan 24, 2022

View reviewed changes

cylc/flow/cfgspec/globalcfg.py Outdated Show resolved Hide resolved

cylc/flow/scripts/config.py Outdated Show resolved Hide resolved

wxtim and others added 4 commits January 25, 2022 10:58

Update cylc/flow/scripts/config.py

e76acc3

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Apply suggestions from code review

9623512

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

refactor testing

8d568fc

Merge branch 'cylc_config_-_add_--platform-names' of github.com:wxtim…

4a6ad5d

…/cylc into cylc_config_-_add_--platform-names * 'cylc_config_-_add_--platform-names' of github.com:wxtim/cylc: Apply suggestions from code review Update cylc/flow/scripts/config.py

Update cylc/flow/scripts/config.py

581dbde

oliver-sanders modified the milestones: cylc-8.0.0, cylc-8.0rc1 Jan 25, 2022

hjoliver requested changes Jan 25, 2022

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

cylc/flow/cfgspec/globalcfg.py Outdated Show resolved Hide resolved

wxtim requested a review from hjoliver January 26, 2022 13:11

wxtim and others added 3 commits January 26, 2022 13:20

response to review

07e7d9e

Merge branch 'cylc_config_-_add_--platform-names' of github.com:wxtim…

872cfee

…/cylc into cylc_config_-_add_--platform-names * 'cylc_config_-_add_--platform-names' of github.com:wxtim/cylc: Update cylc/flow/scripts/config.py

Apply suggestions from code review

840e689

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

wxtim and others added 7 commits January 26, 2022 15:33

created cylc config --platform-names and --platform-meta options; Mov…

f489398

…ed tests/cylc-get-config to tests/cylc-config

refactor testing

ba77857

Update cylc/flow/scripts/config.py

1454774

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Apply suggestions from code review

f7415e8

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

response to review

6f5ac93

Apply suggestions from code review

38b5a26

Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>

Fix func test.

030cd94

Refactor config options checking a bit.

341f7d4

hjoliver force-pushed the cylc_config_-_add_--platform-names branch from ae211d3 to 341f7d4 Compare January 27, 2022 02:04

hjoliver approved these changes Jan 27, 2022

View reviewed changes

hjoliver merged commit d3c8c48 into cylc:master Jan 27, 2022

wxtim deleted the cylc_config_-_add_--platform-names branch January 27, 2022 08:47

MetRonnie pushed a commit to MetRonnie/cylc-flow that referenced this pull request Jan 27, 2022

Merge pull request cylc#4576 from wxtim/cylc_config_-_add_--platform-…

572a5b7

…names Cylc config add `--platform-names`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cylc config add `--platform-names` #4576

Cylc config add `--platform-names` #4576

wxtim commented Jan 12, 2022 •

edited by oliver-sanders

Loading

oliver-sanders left a comment

oliver-sanders commented Jan 20, 2022

wxtim commented Jan 20, 2022

oliver-sanders commented Jan 21, 2022 •

edited

Loading

MetRonnie left a comment

MetRonnie Jan 24, 2022

wxtim Jan 25, 2022

MetRonnie Jan 25, 2022

wxtim Jan 26, 2022

MetRonnie Jan 26, 2022

hjoliver Jan 27, 2022

hjoliver left a comment •

edited

Loading

wxtim commented Jan 26, 2022

MetRonnie commented Jan 26, 2022

oliver-sanders commented Jan 26, 2022 •

edited

Loading

MetRonnie commented Jan 26, 2022 •

edited

Loading

wxtim commented Jan 26, 2022

hjoliver commented Jan 26, 2022 •

edited

Loading

hjoliver commented Jan 27, 2022 •

edited

Loading

hjoliver left a comment

Cylc config add --platform-names #4576

Cylc config add --platform-names #4576

Conversation

wxtim commented Jan 12, 2022 • edited by oliver-sanders Loading

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders commented Jan 20, 2022

wxtim commented Jan 20, 2022

oliver-sanders commented Jan 21, 2022 • edited Loading

MetRonnie left a comment

Choose a reason for hiding this comment

MetRonnie Jan 24, 2022

Choose a reason for hiding this comment

wxtim Jan 25, 2022

Choose a reason for hiding this comment

MetRonnie Jan 25, 2022

Choose a reason for hiding this comment

wxtim Jan 26, 2022

Choose a reason for hiding this comment

MetRonnie Jan 26, 2022

Choose a reason for hiding this comment

hjoliver Jan 27, 2022

Choose a reason for hiding this comment

hjoliver left a comment • edited Loading

Choose a reason for hiding this comment

wxtim commented Jan 26, 2022

MetRonnie commented Jan 26, 2022

oliver-sanders commented Jan 26, 2022 • edited Loading

MetRonnie commented Jan 26, 2022 • edited Loading

wxtim commented Jan 26, 2022

hjoliver commented Jan 26, 2022 • edited Loading

hjoliver commented Jan 27, 2022 • edited Loading

hjoliver left a comment

Choose a reason for hiding this comment

Cylc config add `--platform-names` #4576

Cylc config add `--platform-names` #4576

wxtim commented Jan 12, 2022 •

edited by oliver-sanders

Loading

oliver-sanders commented Jan 21, 2022 •

edited

Loading

hjoliver left a comment •

edited

Loading

oliver-sanders commented Jan 26, 2022 •

edited

Loading

MetRonnie commented Jan 26, 2022 •

edited

Loading

hjoliver commented Jan 26, 2022 •

edited

Loading

hjoliver commented Jan 27, 2022 •

edited

Loading