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.

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

Merged
merged 16 commits into from
Aug 24, 2017

Conversation

challurip
Copy link
Contributor

meta section of suite contains title, description, URL and other user-defined items about suite.

@hjoliver hjoliver added this to the next release milestone Aug 10, 2017
@hjoliver
Copy link
Member

hjoliver commented Aug 10, 2017

(Not essential for next release, but this goes nicely with the runtime [meta] change).

@hjoliver hjoliver self-requested a review August 10, 2017 04:42
@hjoliver
Copy link
Member

@challurip - I've put up a PR to your branch to fix the scan tests.

@hjoliver
Copy link
Member

@challurip - another merge from master should make the authentication tests pass on Travis CI now.

Copy link
Member

@hjoliver hjoliver left a 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?

@@ -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
Copy link
Member

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?

Copy link
Contributor Author

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)

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

nice

@hjoliver
Copy link
Member

@oliver-sanders - can you review or re-assign, while Matt is away?

@hjoliver hjoliver changed the title General items of suite are now in meta section Put suite metadata items in a meta section Aug 15, 2017
Copy link
Member

@hjoliver hjoliver left a 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

Copy link
Member

@oliver-sanders oliver-sanders left a 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:

if key == "URL":
handler_data["suite_url"] = quote(value)
else:
handler_data["suite_"+key] = quote(value)
Copy link
Member

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?

@@ -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:
Copy link
Member

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 ...

\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:
Copy link
Member

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:

@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

It can also include ...

Copy link
Member

Choose a reason for hiding this comment

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

... from the [meta] section...

Copy link
Member

@hjoliver hjoliver left a 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.

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
Copy link
Member

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')

Copy link
Member

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)


\subsubsection[title]{ [meta] \textrightarrow title}

A single line description of the suite. It is displayed in the db viewer
Copy link
Member

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'.

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.
Copy link
Member

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".

@@ -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:
Copy link
Member

Choose a reason for hiding this comment

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

... from the [meta] section...

@hjoliver
Copy link
Member

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, [[[meta]]] should collapse and expand like [[[job]]] etc.

@hjoliver
Copy link
Member

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.:

   [[[events]]]
        failed handler = send-help.sh %(suite_title)s %(suite_importance)s %(title)s ...

@hjoliver
Copy link
Member

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.

Copy link
Member

@hjoliver hjoliver left a 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!

[[root]]
[[[events]]]
failed handler = send-help.sh %(suite\_title)s %(suite\_importance)s %(title)s
\end{lstlisting}
Copy link
Member

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).

\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".
Copy link
Member

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 ``...'')

@@ -1234,7 +1267,17 @@ \subsection{[runtime]}
needs. For example, the value of an "importance" item could determine how an event
Copy link
Member

Choose a reason for hiding this comment

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

quotes again here

@@ -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\_\_}
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants