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

Fix cylc review documentation. #2924

Merged
merged 5 commits into from
Jan 16, 2019

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Jan 15, 2019

Close #2896

Fix several errors is our new-ish Cylc Review documentation:

  • it says "you can open the Cylc Review page for a suite by running the cylc review command.", which isn't true for either the WSGI or ad hoc service.
  • the configuration docs point to lib/cylc/review.py as the application instead of bin/cylc-review
  • Apache directory access configuration was omitted
  • WSGI configuration is documented in the Tutorial section

@hjoliver hjoliver added the bug label Jan 15, 2019
@hjoliver hjoliver self-assigned this Jan 15, 2019
@hjoliver
Copy link
Member Author

hjoliver commented Jan 15, 2019

Docs rewritten; TODO: move 'em out of the Tutorial section.

@hjoliver
Copy link
Member Author

@matthewrmshin - the ad hoc server's status file is not deleted on shutdown, is that the intended behaviour?

@hjoliver
Copy link
Member Author

hjoliver commented Jan 15, 2019

Also, regarding check-box 1 above: I have just removed that documentation, but was the original intention for cylc review to open the web view for the current suite (a la rose suite-log) as well as start the ad hoc service?

@hjoliver
Copy link
Member Author

Ping @sadielbartholomew - just to note this (once I've finished) will have to be merged with #2910 (which shouldn't be hard; but I wanted to get on to this quickly as the errors might stop others using cylc review).

@matthewrmshin matthewrmshin added the doc Documentation label Jan 15, 2019
@matthewrmshin matthewrmshin added this to the next-release milestone Jan 15, 2019
Copy link
Contributor

@matthewrmshin matthewrmshin left a comment

Choose a reason for hiding this comment

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

Although I have not tested Cylc Review on other WSGI servers, I cannot see why WSGI mode would not work as long as a server supports WSGI, so it may be better if we can generalise our statements regarding Apache + mod_wsgi.

doc/src/cylc-user-guide/cug.tex Outdated Show resolved Hide resolved
doc/src/cylc-user-guide/cug.tex Outdated Show resolved Hide resolved
@sadielbartholomew
Copy link
Collaborator

sadielbartholomew commented Jan 15, 2019

Fix several errors is our new-ish Cylc Review documentation

Just to say that I am sorry if there were a number of errors (as here & in #2889) in the initial text describing Cylc Review 😞. When I added in that content I simply lifted & shifted the text from the Rose docs, only tweaking explicit references to Rose (& missing some). I didn't check to see if the instructions were still valid as I assumed there would be no behaviour or set-up change, & it was a tight deadline to get these added before the imminent release. I should have been more thorough though.

@kinow
Copy link
Member

kinow commented Jan 15, 2019

Just looking at the size of the changes for cylc review and the new docs, and the time you had to work on both issues, I think you did an excellent work! There's always going to have some issues left. But that's what we are here for :) (and I managed to try cylc review for the first time following the docs you prepared, and I found them super useful, only missing nginx which I was using)

@sadielbartholomew
Copy link
Collaborator

Thanks @kinow, that is kind of you. It is reassuring that you are all thorough reviewers so that we catch any slip-ups & they don't trickle down to be read & picked up by users 😌.

@hjoliver
Copy link
Member Author

@sadielbartholomew - don't worry about, a few errors are pretty much inevitable, and I figured it was mainly a cut-n-paste from Rose docs. Largely my fault for not noting the doc location and only testing with the ad hoc server in my initial review (lazy of me!).

@hjoliver
Copy link
Member Author

All done. I've moved the configuration section into the installation section, and left some info on usage in the tutorial. Ideally we would cover this sort of thing under Running Suites too, not just the tutorial, but will leave that for an eventual major User Guide rewrite ...

Copy link
Collaborator

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

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

Content revised correctly, & much improved.

There's a minor issue (highlighting a CLI 'bug' of sorts), but since I need to convert these changes to allow #2910 to be merged I will approve this, merge it, & open up an Issue for the CLI argument behaviour.

Use the Apache log at e.g. \lstinline=/var/log/httpd/= to debug problems.
The service should start at \lstinline=http://<server>:8080= (the port number
can optionally be set on the command line). Service logs are written to
\lstinline=~/.cylc/cylc-review*=. Run \lstinline=cylc review status= to view
Copy link
Collaborator

Choose a reason for hiding this comment

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

Strictly the status command should simply be cylc review, not cylc review status. This is as documented under the docstring & output:

$ cylc review --help                 
Usage: cylc [info] review [OPTIONS] [start [PORT]] [stop] 
...
With no arguments, the status of the ad-hoc web service server is printed.
...

I had a play around & cylc review status does actually produce the same result, but only because any argument (or precisely any number not too many i.e. <=2) other than the valid stop or start will not be recognised so that the logic applied for no arguments is run. E.g.:

$ cylc review any nonsense
host=0.0.0.0
pid=7395
port=8080

Copy link
Collaborator

Choose a reason for hiding this comment

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

For context, this originates from Rose Bush. Running after starting it as an ad-hoc server:

$ rose bush anything really
host=0.0.0.0
pid=13839
port=4100

@sadielbartholomew sadielbartholomew merged commit b11f9f7 into cylc:master Jan 16, 2019
@hjoliver hjoliver deleted the fix-cylc-review-docs branch January 16, 2019 09:54
sadielbartholomew added a commit to sadielbartholomew/cylc-flow that referenced this pull request Jan 16, 2019
sadielbartholomew added a commit to sadielbartholomew/cylc-flow that referenced this pull request Jan 16, 2019
sadielbartholomew added a commit to sadielbartholomew/cylc-flow that referenced this pull request Jan 16, 2019
sadielbartholomew added a commit to sadielbartholomew/cylc-flow that referenced this pull request Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug doc Documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants