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

Refactor docs #156

Merged
merged 15 commits into from
Jul 14, 2021
Merged

Refactor docs #156

merged 15 commits into from
Jul 14, 2021

Conversation

colearendt
Copy link
Member

Split READMEs into separate per-directory READMEs
Create table of contents for the main README.md

@colearendt colearendt requested review from akgold and bdeitte July 9, 2021 18:41
Copy link
Contributor

@akgold akgold left a comment

Choose a reason for hiding this comment

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

I love where this is headed. A bunch of nitpicky comments.

One other thing -- should we mention somewhere that one bit of "special sauce" in here is the script to release the license on container kill and people should be sure to keep that bit if they're editing the files for their own use?

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
connect/README.md Outdated Show resolved Hide resolved
connect/README.md Outdated Show resolved Hide resolved
connect/README.md Outdated Show resolved Hide resolved
server-pro/README.md Outdated Show resolved Hide resolved
server-pro/README.md Outdated Show resolved Hide resolved
server-pro/README.md Outdated Show resolved Hide resolved
server-pro/README.md Outdated Show resolved Hide resolved
| `RSP_LICENSE` | License key for RStudio Server Pro, format should be: `XXXX-XXXX-XXXX-XXXX-XXXX-XXXX-XXXX` | None |
| `RSP_LICENSE_SERVER` | Floating license server, format should be: `my.url.com:port` | None |
| `RSP_LAUNCHER` | Whether or not to use launcher locally / start the launcher process | true |
| `RSP_LAUNCHER_TIMEOUT` | The timeout, in seconds, to wait for launcher to startup before proceeding | 10 |
Copy link
Contributor

Choose a reason for hiding this comment

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

This is upon container start? If so...

Suggested change
| `RSP_LAUNCHER_TIMEOUT` | The timeout, in seconds, to wait for launcher to startup before proceeding | 10 |
| `RSP_LAUNCHER_TIMEOUT` | Number of seconds to pause container startup for the launcher process to start | 10 |

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understood quite right what this is...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's not quite "pause container startup." This is basically how long to wait for the launcher to start. Usually it is very quick. If it does not succeed in this amount of time, then the container startup will fail / error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried to add a bit more explanation

Copy link
Contributor

Choose a reason for hiding this comment

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

Aha -- maybe it'd be clearer to say the timeout is until an error...because it's not a "wait to proceed", it's a "wait to fail", right?

What about "The time, in seconds, the container waits for the launcher process to start before throwing an error"?

Copy link
Member Author

Choose a reason for hiding this comment

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

So it's not necessarily a "wait to error." It's very hard / tricky to tell if launcher failed (i.e. it could just sit there doing nothing forever). It is literally "wait X seconds, if you don't get a listening service by then, presume failure"

Copy link
Contributor

Choose a reason for hiding this comment

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

"The time, in seconds, to wait for a listening launcher process before throwing an error"?

Copy link
Contributor

Choose a reason for hiding this comment

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

This really isn't a big deal 😆. I'm sure it's fine as is. I'm just persnickety about writing.

Co-authored-by: Alex Gold <alex.gold@rstudio.com>
package-manager/README.md Outdated Show resolved Hide resolved
package-manager/README.md Outdated Show resolved Hide resolved
server-pro/README.md Outdated Show resolved Hide resolved
server-pro/README.md Outdated Show resolved Hide resolved
@colearendt colearendt requested a review from akgold July 14, 2021 13:38
Copy link
Contributor

@akgold akgold left a comment

Choose a reason for hiding this comment

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

Looks great. A bunch of small copy edits and a few remaining things. No need for me to see again.

helper/float/README.md Show resolved Hide resolved
helper/float/README.md Outdated Show resolved Hide resolved
package-manager/README.md Outdated Show resolved Hide resolved
package-manager/README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
server-pro/README.md Outdated Show resolved Hide resolved
server-pro/README.md Outdated Show resolved Hide resolved
server-pro/README.md Outdated Show resolved Hide resolved
server-pro/README.md Outdated Show resolved Hide resolved
#### Users

By default, the container will create a test user, which you can control or disable with the environment
variables: `RSP_TESTUSER`, `RSP_TESTUSER_PASSWD`, `RSP_TESTUSER_UID`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume renaming from RSP will be a part of the workbench conversion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's a good call... breaking, unfortunately, but we haven't published the workbench images widely yet 🤷‍♂️ (or documented publicly)


**WARNING**: Floating Licenses should not be used within docker containers in a production context, since the docker
container failing could require manual intervention to fix (see the app below). We provide these images only for
development and testing.
Copy link
Member Author

@colearendt colearendt Jul 14, 2021

Choose a reason for hiding this comment

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

@akgold Does this warning not suffice? Not clear enough? There is another one down below

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok. I'd probably vote to go even a little stronger. I struggle to see a situation where customers should use these at all if they're not in direct conversation with us.

Like...I think our preferred recommendation would be to set up the license server, then start playing with the rest of Team.

Copy link
Member Author

Choose a reason for hiding this comment

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

For sure. I agree 😄 I'll make it stronger

colearendt and others added 4 commits July 14, 2021 10:02
Co-authored-by: Alex Gold <alex.gold@rstudio.com>
Co-authored-by: Alex Gold <alex.gold@rstudio.com>
@colearendt colearendt merged commit 3df40ca into main Jul 14, 2021
@colearendt colearendt deleted the refactor-docs branch July 14, 2021 14:36
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.

None yet

2 participants