-
Notifications
You must be signed in to change notification settings - Fork 93
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
Platforms: Upgrade Cylc 7 Host Syntax #3772
Platforms: Upgrade Cylc 7 Host Syntax #3772
Conversation
9c89175
to
bf70a1c
Compare
@@ -221,9 +221,13 @@ def test_upgrader_where_host_is_function(tmp_path, caplog): | |||
caplog.set_level(logging.DEBUG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this file has been moved to functional/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that at one point I began to believe that this was more functional than unit, but it isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what aspect of this test is functional?
The "line in the sand" that I have in my head is:
- Integration - Involves the interaction of more than one significant piece of logic.
- Functional - Involves interaction with the shell, browser or other systems.
This is why the integration test suites are started in held mode and don't involve anything that goes via the CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has been moved back again or removed?
f51a918
to
ee35376
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to go through and update *.rc -> .cylc
(A few conflicts and test failures now). |
9daf8aa
to
e817122
Compare
804b815
to
ea51631
Compare
replace platform_from_name with get_platform where args=None replace platform_from_name with get_platform where args!=None fix unit tests with new syntax
allow for platform = localhost where to settings at cylc 7 or 8 have been set removed unwanted tests for deleted method finessing the platform lookup
…rt.t tests: tests/functional/cylc-get-config/00-simple/section2.stdout tests: tests/f/job-submission/10-at-shell.t tests: tests/functional/directives/01-at.t tests: tests/functional/job-submission/00-user.t
dced080
to
64d0265
Compare
aaa3f93
to
326f3fe
Compare
Can I poke @oliver-sanders and @MetRonnie to have a final double check that I've answered outstanding issues? |
You said that I couldn't do the upgrade before inheritance, but that I didn't want to warn for every failure after inheritance. So I've replaced the upgrade-on-config-loading function with a warn (fail if is deffo wrong) at config-load, then added a the upgrade logic at task job sumbit.
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>
# ---------------------------------------------------------------------------- | ||
# Tests of platform_from_name | ||
# ---------------------------------------------------------------------------- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker but we shouldn't have file-dividers in code. If the file is getting so big that you feel the need to add markers to divide related logic into separate sections, consider splitting the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've always hated them in the bash test scripts too 🤮 (no offense @wxtim !)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None Taken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new upgrade logic seems a pretty good way to do this. 👍
a9551a4
to
4bb8e9c
Compare
4bb8e9c
to
2122c51
Compare
b52428e
to
93522d0
Compare
I think all outstanding comments are resolved, looks ok, merging with two approvals. |
Summary
As per the deprecation method section of the platforms proposal suite tasks where Cylc 7 syntax such as
should select a sensible batch system, or return a reasonable error if unable to do so.
This PR attempts to address this.
Work done
cylc.flow.cfgspec.suite.host_to_platform_upgrader
and replaced it with an upgrade warner function to be called on workflow validation. This can't catch every possible problem, but will hopefully fail loudly if something obviously wrong is put in a config.get_platform
interface to the platform module: Replaced instances ofplatform_from_name
withget_platform
.get_platform
.tests/functional/validate/04-host-platform-upgrade.t
- this is not meant to comprehensively test the upgrader function, merely to test that it has been plugged into the cylkctests/unit/cfgspec/test_suite.py
which is meant to provide comprehensive coverage of the warning function, and a unit test forget_platform
totests/unit/test_platforms.py
.