-
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
Supply URLs to suite and task event handlers. #2276
Conversation
@@ -316,9 +320,6 @@ def __init__( | |||
self.TABLE_TASK_STATES: [], | |||
} | |||
|
|||
# TODO - should take suite name from config! | |||
self.suite_name = os.environ['CYLC_SUITE_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.
Doesn't need to be an instance variable, and this was an insane way of getting the suite name.
@@ -23,7 +23,7 @@ OPT_SET= | |||
if [[ "${TEST_NAME_BASE}" == *-globalcfg ]]; then | |||
create_test_globalrc "" " | |||
[task events] | |||
handlers=hello-event-handler '%(name)s' '%(event)s' | |||
handlers = hello-event-handler %(name)s %(event)s %(suite_url)s %(task_url)s %(message)s %(point)s %(submit_num)s %(id)s |
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.
Might as well test all possible args.
Will be very minor conflicts with 2157 |
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.
Please consider modifying url to URL in the docs. Looks OK in general, and should be relatively straightforward to merge/port into #2157.
doc/src/cylc-user-guide/suiterc.tex
Outdated
@@ -1546,6 +1547,8 @@ \subsection{[runtime]} | |||
\item \%(suite)s: suite name | |||
\item \%(point)s: cycle point | |||
\item \%(name)s: task name | |||
\item \%(suite_url)s: suite url | |||
\item \%(task_url)s: task url |
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.
Should be URL (ALL CAPS), to be consistent with line 304 above.
@oliver-sanders please sanity check. I am tempted to just merge this and have the docs fixed when I resolve the conflicts in #2157. |
Changed 'url' to 'URL' in the user guide. |
Fixes following merge with cylc#2276.
Close #2275
Not for review yet - just needs tests.Tests added (or rather, existing tests upgraded).