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.

Sign up for GitHub

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

Jump to bottom

Expand [platforms] section; Add warning if a platform name regex matches localhost #4854

Merged
merged 10 commits into from
May 13, 2022
Merged

Expand [platforms] section; Add warning if a platform name regex matches localhost #4854

merged 10 commits into from
May 13, 2022

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented May 4, 2022 •
edited by oliver-sanders
Loading

These changes close #4845

[post-hoc edit to ticket for the benefit of users clicking on changelog]

Summary

Expand Platform definitions:

Allow platform definitions to use comma separated lists and be expanded as task definitions are. E.g

[[foo, bar]]
  hosts = host1, host2
[[foo]]
  install target = host2

is now equivelant to

[[foo]]
  hosts = host1, host2
  install target = host2
[[bar]]
  hosts = host1, host2

Fail if "localhost" matches a platform definition Regex.

Because "localhost" platform is a default item it is always the last platform to be checked. If any platform definition is a regex which matches "localhost" then that definition will be used instead. To prevent this, bad such platform definitions.

Example

In the following example localhost|foo|bar... would have always been selected before this change, but will now be rejected.

[platforms]
  [[localhost|foo|bar...]]
    # some settings
  [[localhost]]
    # other settings

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).
  • Change log entry added.
  • No documentation update required.

@wxtim wxtim self-assigned this May 4, 2022
@wxtim wxtim added this to the cylc-8.0rc4 milestone May 4, 2022
@wxtim wxtim requested review from datamel and oliver-sanders May 4, 2022 14:29
datamel
datamel approved these changes May 5, 2022
View reviewed changes
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

Working as expected for me. Checked out and test run. Can't spot any implementation problems.
Approving on proviso that the purge is sorted in the test :) Thanks @wxtim

@wxtim
Copy link
Member Author

wxtim commented May 5, 2022 •
edited
Loading

Make it an exception.
Add a changelog.

@wxtim wxtim marked this pull request as draft May 5, 2022 14:22
@oliver-sanders
glblcfg: split comma separated platform definitions
1c20cd5 
* Partially addresses cylc#4845
* This is intended to make it easier to configure the localhost platform
  to match other hosts e.g
  `[platform][localhost, desktop..., server...]`
* Note this **cannot** be done using a regex e.g.
  `[platform][localhost|desktop...|server...]` because the localhost
  platform is special (it gets defined by default as [localhost]) so the
  regex is not matched and consequently gets ignored - cylc#4845
@wxtim wxtim marked this pull request as ready for review May 6, 2022 08:13
@wxtim wxtim marked this pull request as draft May 6, 2022 13:43
@wxtim wxtim marked this pull request as ready for review May 6, 2022 14:47
@wxtim
Merge branch 'expand_platforms_section' into warn.global-config-conta…
71a8afd 
…ins-localhost

* expand_platforms_section:
  glblcfg: split comma separated platform definitions
  tests/u: parsec/test_util unittest->pytest
  bump deprecation warnings from Cylc 9 to 8.x (cylc#4853)
  Update tests/functional/execution-time-limit/04-polling-intervals.t
  Stop a few deprecation warnings in Cylc7 back compat mode
  Add tests
  Add polling interval test and change log entry.
  Set excecution polling delays correctly
@wxtim wxtim marked this pull request as draft May 6, 2022 14:49
@wxtim wxtim requested a review from datamel May 6, 2022 15:29
@wxtim wxtim marked this pull request as ready for review May 6, 2022 15:29
@wxtim
Copy link
Member Author

wxtim commented May 6, 2022

@oliver-sanders Do you want me to count having reviewed your parts of the code as review 1 and you as having reviewed mine as review 1 so we only want 1 further reviewer, or should I/we tag Ronnie?

@datamel This is not the PR you approved any more.

@oliver-sanders
Copy link
Member

Do you want me to count having reviewed your parts of the code as review 1 and you as having reviewed mine

Sounds good.

@oliver-sanders
Copy link
Member

oliver-sanders commented May 9, 2022 •
edited
Loading

Shellcheck is complaining about syntax in a comment (false positive).

Oh wait, it's not a false positive because it's in double quotes. If you change the quotes used by create_test_global_config to single that'll do it.

@wxtim wxtim requested a review from oliver-sanders May 11, 2022 10:21
@oliver-sanders
Copy link
Member

Ooh good catch by shellcheck - SC1011: This apostrophe terminated the single quoted string!

@wxtim
Better handling of [platforms][localhost]
242101f 
convert warning to outright error

ensure that failure occurs wherever in hte platforms list the inappropriate regex is.

Update tests/functional/platforms/08-warn-if-regex-matches-localhost.t

Co-authored-by: Oliver Sanders <[email protected]>

removed two tests which will fail (correctly) after changes
@wxtim wxtim force-pushed the warn.global-config-contains-localhost branch from b13a574 to 242101f Compare May 12, 2022 09:41
@wxtim
Merge branch 'warn.global-config-contains-localhost' of github.com:wx…
5abbd63 
…tim/cylc into warn.global-config-contains-localhost

* 'warn.global-config-contains-localhost' of github.com:wxtim/cylc:
  changelog
  Remove setting of execution timeout based on execution time limit
  jinja2: improve error context information
  Update cylc/flow/workflow_files.py
  contact: process check should handle dirty JSON
@wxtim wxtim requested a review from oliver-sanders May 12, 2022 14:20
datamel
datamel approved these changes May 13, 2022
View reviewed changes
Copy link
Contributor

@datamel datamel left a comment

Choose a reason for hiding this comment

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

I've checked this out and read the code. Tests pass locally for me. No problems found from me. Thanks @wxtim

@datamel datamel merged commit 89f45eb into cylc:master May 13, 2022
@dpmatthews dpmatthews modified the milestones: cylc-8.0rc4, cylc-8.0rc3 May 13, 2022
@wxtim wxtim deleted the warn.global-config-contains-localhost branch May 17, 2022 14:35
@MetRonnie MetRonnie mentioned this pull request May 18, 2022
Merged
7 tasks
@wxtim wxtim changed the title Add warning if a platform name regex matches localhost Expand [platforms] section; Add warning if a platform name regex matches localhost May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@datamel datamel datamel approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@wxtim wxtim

Labels
None yet
Projects
None yet
Milestone
cylc-8.0rc3
Development

Successfully merging this pull request may close these issues.

[[localhost]] platform selects from top of platforms list
4 participants
@wxtim @oliver-sanders @datamel @dpmatthews