-
Notifications
You must be signed in to change notification settings - Fork 94
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
Put suite metadata items in a meta section #2387
Conversation
Modified tests which may get affected
Deprecated suite items
(Not essential for next release, but this goes nicely with the runtime |
@challurip - I've put up a PR to your branch to fix the scan tests. |
@challurip - another merge from master should make the authentication tests pass on Travis CI now. |
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.
Looks good, tests all passing now. One more thing would be nice: could you modify tests/events/20-suite-event-handlers
to test that a custom metadata item such as suite-priority = HIGH
or similar, can be passed to an event handler as expected?
lib/cylc/suite_srv_files_mgr.py
Outdated
@@ -540,7 +540,6 @@ def _get_suite_title(self, reg): | |||
match = re.match('^\s*title\s*=\s*(.*)\s*$', line) | |||
if match: | |||
title = match.groups()[0].strip('"\'') | |||
break |
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.
Did you mean to delete this line?
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.
Yes. It was not there before, and I'm not sure if I can break once the match is found ( as if title is matched then there is no need to continuing and matching other lines, but was not sure if that affects anything else so removed)
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.
nice
@oliver-sanders - can you review or re-assign, while Matt is away? |
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.
Re-reviewing! @challurip - could you please:
- merge my new small pull request to your branch
- update the suite.rc reference in the user guide for these meta items
- and finally, we should make all suite meta items available to task event handlers, as
%(suite_ITEM)s
as we do already for suite URL (and update documentation accordingly). So in task event handlers, we would allow suite_URL, as well as the backward compat suite_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.
Nearly there! Some minor comments:
lib/cylc/task_events_mgr.py
Outdated
if key == "URL": | ||
handler_data["suite_url"] = quote(value) | ||
else: | ||
handler_data["suite_"+key] = quote(value) |
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 this statement be outside the else
so that the suite_url
is accessible as suite_URL
?
doc/src/cylc-user-guide/suiterc.tex
Outdated
@@ -1624,12 +1624,18 @@ \subsection{[runtime]} | |||
\item \%(message)s: event message, if any | |||
\end{myitemize} | |||
|
|||
Can also include the patterns from [[[meta]]] section of the task, for example: | |||
Can include the patterns from [[[meta]]] section of the task, for example: |
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 template can also include the patterns ...
doc/src/cylc-user-guide/suiterc.tex
Outdated
\begin{myitemize} | ||
\item \%(importance)s: task priority | ||
\item \%(color)s: task color | ||
\end{myitemize} | ||
|
||
Also able to include patterns from [meta] section of the suite, for example: |
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.
Patterns from the [meta] section of the suite can also be used, for example:
doc/src/cylc-user-guide/suiterc.tex
Outdated
@@ -315,6 +342,12 @@ \subsection{[cylc]} | |||
\item \%(message)s: event message, if any | |||
\end{myitemize} | |||
|
|||
Can also include the patterns from [meta] section of the suite, for example: |
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 can also include ...
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.
... from the [meta] section...
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.
Just a few minor doc changes.
doc/src/cylc-user-guide/suiterc.tex
Outdated
Section containing metadata items for this suite. Several items | ||
(title, description, URL) are pre-defined and are used by the GUI. Others can be | ||
user-defined and passed to suite event handlers to be interpreted according to your | ||
needs. For example, the value of an "suite-priority" item could determine how an event |
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.
'a "suite-priorty" item' (not 'an')
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 also text quotes in LaTeX are `` (two left single quotes) then '' (two right single quotes)
doc/src/cylc-user-guide/suiterc.tex
Outdated
|
||
\subsubsection[title]{ [meta] \textrightarrow title} | ||
|
||
A single line description of the suite. It is displayed in the db viewer |
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 your fault, but "db viewer" is long outdated. Can you change that to 'displayed in the GUI "Open Another Suite" window'.
doc/src/cylc-user-guide/suiterc.tex
Outdated
window and can be used as a way of grouping related suites together. | ||
\lstinline=cylc show= command. | ||
A multi-line description of the suite. It can be retrieved by the db viewer | ||
right-click menu, or at run time with the \lstinline=cylc show= command. |
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.
(as above) just write "It can be retrieved at run time with the cylc show command".
doc/src/cylc-user-guide/suiterc.tex
Outdated
@@ -315,6 +342,12 @@ \subsection{[cylc]} | |||
\item \%(message)s: event message, if any | |||
\end{myitemize} | |||
|
|||
Can also include the patterns from [meta] section of the suite, for example: |
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.
... from the [meta] section...
I've also noticed the section level of the task (not suite) meta items isn't right - if you look at the side bar in a PDF viewer, |
And finally, can you document - under the runtime meta section heading - that any suite meta item can now be passed to task event handlers by prefixing the string template item name with "suite_", e.g.:
|
And finally_2: there a bunch of example suites in the User Guide (cug.tex) that need title etc. put under the new meta section. |
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.
A few more doc changes, sorry!
doc/src/cylc-user-guide/suiterc.tex
Outdated
[[root]] | ||
[[[events]]] | ||
failed handler = send-help.sh %(suite\_title)s %(suite\_importance)s %(title)s | ||
\end{lstlisting} |
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.
Underscores shouldn't be escaped with \
inside the lstlisting environment (the backslashes get printed here).
doc/src/cylc-user-guide/suiterc.tex
Outdated
\subsection[\_\_MANY\_\_]{ [meta] \textrightarrow \_\_MANY\_\_} | ||
|
||
Replace \_\_MANY\_\_ with any user-defined metadata item. These, like title, URL, etc. can be passed | ||
to suite event handlers to be interpreted according to your needs. For example, "suite-priority". |
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.
Another quoting error ("..." should be ``...'')
doc/src/cylc-user-guide/suiterc.tex
Outdated
@@ -1234,7 +1267,17 @@ \subsection{[runtime]} | |||
needs. For example, the value of an "importance" item could determine how an event |
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.
quotes again here
doc/src/cylc-user-guide/suiterc.tex
Outdated
@@ -59,6 +70,22 @@ \subsubsection{URL} \label{SuiteURL} | |||
\item {\em example:} \lstinline=http://suites/${CYLC_SUITE_NAME}/index.html= | |||
\end{myitemize} | |||
|
|||
\subsection[\_\_MANY\_\_]{ [meta] \textrightarrow \_\_MANY\_\_} |
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.
This should be a subsubsection, to collapse/expand into the meta section.
meta section of suite contains title, description, URL and other user-defined items about suite.