-
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
added suite state xtrigger for back-compat #5864
added suite state xtrigger for back-compat #5864
Conversation
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.
Seems fine. I've made a comment about explicitly testing the warning, but I'm not super fussed if you want to argue that it's unnecessary.
extended back compat a unit test added deprecation log message refactored implementation flake8 review amends added test for warning text review amends more review ammends Replace icky unit test with functional test (#4) * Replace icky unit test with functional test * Update changelog
bd18c77
to
95d0e59
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.
Tests are failing for a valid reason
XtriggerConfigError: [upstream] xtrigger module 'suite_state' not found
This is because of the change introduced in #5831 where built-in xtriggers are now registered by the cylc.xtriggers
entry point in setup.cfg
Lines 218 to 223 in 22f41e2
# NOTE: Built-in xtriggers | |
cylc.xtriggers = | |
echo = cylc.flow.xtriggers.echo:echo | |
wall_clock = cylc.flow.xtriggers.wall_clock:wall_clock | |
workflow_state = cylc.flow.xtriggers.workflow_state:workflow_state | |
xrandom = cylc.flow.xtriggers.xrandom:xrandom |
I think the best solution is to just add suite_state
to the entry point? cc @oliver-sanders
Yes, that will be needed to register this xtrigger. |
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.
👍
@@ -220,6 +220,7 @@ cylc.xtriggers = | |||
echo = cylc.flow.xtriggers.echo:echo | |||
wall_clock = cylc.flow.xtriggers.wall_clock:wall_clock | |||
workflow_state = cylc.flow.xtriggers.workflow_state:workflow_state | |||
suite_state = cylc.flow.xtriggers.suite_state:suite_state |
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.
Note small conflict with #5452
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.
(No need to do anything unless #5452 gets merged first)
# along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
|
||
# Test that deprecation warnings are printed appropriately for the suite_state | ||
# xtrigger. |
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.
Does this functional test provide anything more than the unit tests below?
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.
It's icky to test the deprecation message logging from within unit tests because it happens at import time, so did this as a functional test. #5864 (comment)
Just failing flake8:
|
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.
That's fine - Sorry I failed to review this in a timely fashion.
closes #5835
The issue
The suite_state xtrigger was renamed to workflow_state, this breaks Cylc 7-8 interoperability.
The solution
I have just added a new xtrigger called suite_state which imports from workflow_state
Check List
CONTRIBUTING.md
and added my name as a Code Contributor.setup.cfg
(andconda-environment.yml
if present).CHANGES.md
entry included if this is a change that can affect users?.?.x
branch.