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

Rose Bush utility migration to cylc: incoming #2614

Merged

Conversation

sadielbartholomew
Copy link
Collaborator

@sadielbartholomew sadielbartholomew commented Mar 28, 2018

Migration of the 'Rose Bush' HTTP-based service. Closes #1052.

This PR consists of purely the migration to a functional Cylc facility & related 'tidying up' i.e. removing code needed for Rose as a distinct system to retrieve required Cylc data. Follow-up work such as updating the docs & upgrading bootstrap will be completed separately.

Note:

  1. Other than three core files, one under bin/ & two under lib/, & test files under tests/, new files are all web-based (HTML, CSS & Javascript) files, under a new directory lib/cylc/cylc-nameless/ with identical structure to that currently in rose/lib/html/ (with files relating to rosie disco removed & only small modifications made otherwise). These are mostly external. Note that jquery.dataTables.js constitutes ~3/4 of the ~20,000 line additions for the PR.
  2. Commits 682b311, edd18fc & b15b25b contain unedited copies of Rose files, to allow relative changes to be seen for aid of review.

Checklist:

Open for review & responding to feedback as it is provided.

@sadielbartholomew sadielbartholomew added this to the soon milestone Mar 28, 2018
@sadielbartholomew sadielbartholomew self-assigned this Mar 28, 2018
@oliver-sanders
Copy link
Member

Travis is none too happy, this might be the problem:

NameError: global name 'pre_select_broadcast_states' is not defined

@sadielbartholomew
Copy link
Collaborator Author

I will start working through the issues picked up Travis CI etc when I return. Thanks for the heads-up.

@hjoliver
Copy link
Member

hjoliver commented Apr 2, 2018

A few name ideas:

  • cylc suite-log - boring but intuitive? (but I guess this will be the equivalent of rose suite-log - i.e. view logs for a given suite, not the name of the service?)
  • cylc slog - short for previous - snappy, but weird and unpleasant?
  • cylc bush - to acknowledge its history, but puzzling to future users?
  • cylc web-log - for obvious reasons
  • cylc blog - short for previous ... but suggests blogging capability? (so, probably not).

Actually, I guess the name of the service does not particularly have to be intuitive or even meaningful, because only cylc suite-log (presumably, see above) will be invoked regularly by users. So we could go for a memorable/interesting/nice name instead. Such as cylc bush ... or cylc moth (allows you to "flutter about among the job logs"... )

@hjoliver
Copy link
Member

hjoliver commented Apr 2, 2018

+21k lines - that's expanded the code base!

@hjoliver
Copy link
Member

hjoliver commented Apr 2, 2018

Noticed the CLI cylc nameless --help has inherited a few options that it doesn't need (e.g. --icp).

@hjoliver
Copy link
Member

hjoliver commented Apr 3, 2018

  • I'll take a look at the icon format
  • regarding Rose config files - can we just provide access to them if found, otherwise not? (or am I missing the point here?)
  • file and class locations seem fine to me at first glance.

@oliver-sanders
Copy link
Member

A few name ideas

In the, hopefully, not too distant future, we hope to incorporate "cylc nameless" into the "one cylc gui" so it would be good to agree on a naming which is compatible with this...

System Name (actions) Name (descriptions)
rose edit (possible plugin) Configure Rose Config
cylc gui Control Live
cylc nameless Review History

I like the idea of a name which clarifies the functionality of cylc gui and cylc nameless as this is a source of confusion for new users.

@oliver-sanders
Copy link
Member

As regards the favicon png is fine, https://en.wikipedia.org/wiki/Favicon#File_format_support

@sadielbartholomew
Copy link
Collaborator Author

sadielbartholomew commented Apr 5, 2018

Thanks for the suggestions & comments so far all :) I'm working on this on & off (& will comment when the tests adaptation is done) & will read any further comments should you think of anything further to add. In response:

The name: Of course it needs to be a joint decision so I think the best way to proceed (if you disagree do say) is for the Metomi team to have a discussion, from which I will summarise the main ideas & points made & report back in a comment here. Hopefully we can narrow our ideas down & try to agree as far as possible as a team before discussing by comment. Though I am glad Oliver commented RE compatibility with future plans as I think that is very important! We will have this discussion sometime next week (as early as possible given people's leave).

Favicon, locations... (anything else not mentioned): great, I will leave these be for now.

@hjoliver Thanks for alerting me about the --help options, I'll sort that out with the tests. And yes, I was simply asking whether the Rose config files should be displayed at all i.e. 'access if found' as you state. I believe Oliver had pointed out that, for example, if 'standard' Rose files are to be recognised and displayed in this way it may require them to be outlined in the cylc documentation. For (similar?) reasons one might have argued not to acknowledge them at all. But it seems we are all happy with the 'access if found' approach (I discussed this in person with Matt & Oliver). A nod to the existing reference to the Rose docs would probably suffice for the point provided.

@sadielbartholomew
Copy link
Collaborator Author

sadielbartholomew commented May 4, 2018

@hjoliver, regarding the discussion of the name - various leave meant this was delayed, but we had a conversation about it yesterday, from which I will summarise:

  • Your own ideas: we thought 'suite-log' would be confusing & 'web-log' inappropriate for future plans given the GUI should replace the web aspect, but simply 'log' could be good. We weren't keen on the others.

  • The future vision for the GUI was our main consideration. We envision a set of names for the function elements, all either verbs or nouns providing intuitive descriptions or actions for each. We lean towards action verbs & are thinking along the lines of:

    Configuration Graphing 'Rose Bush' Analytics
    configure preview ('cylc graph') /control ('gcylc') review analyze
  • So we have 'review' as our main suggestion.

Let us know your thoughts on the above.

@hjoliver
Copy link
Member

hjoliver commented May 7, 2018

@sadielbartholomew - yes, I like those suggestions. Not entirely sure of "review" but I haven't thought of anything better.

@sadielbartholomew
Copy link
Collaborator Author

Thanks @hjoliver, I will bring this up again in our team meeting tomorrow & mention your comment. If we have any more thoughts I will convey them here. There is no particular urgency on the naming decision yet, as I believe we are still waiting on the legal advice & after I fix the tests & any related issues (will get back onto this soon) there is still the review stage which may take a while.

@sadielbartholomew
Copy link
Collaborator Author

Re-started work on this PR this afternoon.

There are a large number of issues (250+) as highlighted by Codacy, but almost all are from the web-based files & inherited from the original Rose code. Only a small number are in the Python code & I have removed all of the rectifiable Python issues. So (for this PR at least) we should ignore the Codacy failure.

I do obviously however need to resolve the failures picked up by Travis CI / the test battery & will do that next.

@oliver-sanders
Copy link
Member

There is a minor problem with some of the text in the current rose bush it says:

Cycles (before, after or patterns): >CYCLE | <CYCLE | GLOB ...

When it should be:

Cycles (before, after or patterns): <CYCLE | >CYCLE | GLOB ...

Could we possibly sneak this one-line change in with this PR 🙏.

@sadielbartholomew
Copy link
Collaborator Author

Sure, I'll add that to the checklist. Hope to get through all or most of the items on it today :)

@sadielbartholomew sadielbartholomew force-pushed the rose-bush-migration-incoming branch 7 times, most recently from c800a12 to 11236c6 Compare July 2, 2018 15:02
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@cylc cylc deleted a comment Oct 29, 2018
@sadielbartholomew
Copy link
Collaborator Author

We will need to reference this code to the cylc ACKNOWLEDGEMENT.md file

Thanks @oliver-sanders, good thinking. I've just done that & squashed it into the previous commit. It was a copy & paste apart from reformatting them in line with the Cylc acknowledgement file, adapting the paths to those now in Cylc, & also (note) changing the Bootstrap version link to match that referenced (checking the tweaked one works).

Brace yourselves - Codacy is about to spam us again!

@sadielbartholomew
Copy link
Collaborator Author

For information, Oliver spotted an issue relating to broadcast events, which he fixed & I have merged in (my bad, this was hard to test as I struggled to find suites with broadcasts). Otherwise, good to go (again)!

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.

🏅 Approved 🏅

@oliver-sanders
Copy link
Member

I feel like there should be some ceremony, 🍾 ➡️ ⛵.

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.

6 participants