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.

Sign up for GitHub

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

Use opensafely launch rstudio to provide rstudio #92

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use opensafely launch rstudio to provide rstudio #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions DEVELOPERS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,20 @@ just # shortcut for just --list
## Build instructions

Run `just build` to build the Docker image.

## Local vscode testing

Simplest way to test is to clone the research-template, which will have the
bloodearnest marked this conversation as resolved.
Show resolved Hide resolved
latest devcontainer set up.

```
git clone https://github.com/opensafely/research-template
```

You will then need to edit its .devcontainer/devcontainer.json to use just
"research-template", rather than the fully qualified ghcr.io/... name. This
will now use your locally build image.

You can then run `code .` in that directory, and then Ctrl-Shift-P, type
"rebuild", and select "Rebuild With Cache and Open in Container" This will
ensure your devconatainer will use your local image changes.
bloodearnest marked this conversation as resolved.
Show resolved Hide resolved
9 changes: 8 additions & 1 deletion devcontainer/postAttach.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,9 @@
#!/bin/bash
sudo rstudio-server start
set -eu
test -f project.yaml || exit
images=$(opensafely info --list-project-images | sort | uniq)
if [[ "$images" =~ "r:v2" ]]; then
opensafely launch rstudio:v2 --port 8787 --background --no-browser
else
opensafely launch rstudio:v1 --port 8787 --background --no-browser
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need a default case? By my reading of this, if someone starts up a new codespace/dev container with a new project that doesn't yet have an R action defined in the project.yaml then they won't get an RStudio session started.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, for example if they are just using stata or python, there's no point running rstudio?

Re-attaching to (or rebuilding) the codespace after adding an R action to project.yaml will kick off rstudio, AIUI. That does leave an awkward gap though.

Happy to leave it running regardless, I guess that's the minimal change? But I would suggest we default to v2 if no R actions are currently defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have an idea for improving the UX of the general "this codespace/.devcontainer is out of date" problem, with a way to "just update everything".

Which will possibly help with resolving this in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% sure on what's the best way. I think the minimal change is best if you want to keep this moving otherwise we (Team REX) can discuss this beginning of next week.

ISTR that when we were doing the first codespaces initiative we decided that non-R users paying the penalty of all the things needed to start up RStudio was worth it for the convenience of R users as the latter is so massively the majority of our users.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have updated to run v1 by default, unless v2 is in the project.yaml