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

feat: add rstudio #172

Closed
wants to merge 1 commit into from
Closed

Conversation

rokroskar
Copy link
Contributor

Here is an attempt to add an RStudio feature.

I was having some issues getting the no-auth to work, I could use some hints. Basically the browser entered into some sort of redirect loop; with auth it works fine.

Also note that this is using a feature preview of the devcontainers feature spec, namely the dependsOn property so schema checkers complain.

An example devcontainer.json:

{
	"image": "ubuntu",
	"features": {
		"./rstudio": {}
	},
	"remoteUser": "rstudio",
	"portsAttributes": {
		"56559": {
			"label": "RStudio port",
			"onAutoForward": "openBrowser"
		}
	},
	"forwardPorts": [56559],
	"postStartCommand": "rserver --auth-none=0 --www-frame-origin=same --www-port=56559 --www-verify-user-agent=0 --secure-cookie-key-file=/tmp/rstudio-cookie-file --server-user=$(whoami) --database-config-file=/tmp/db-conf/db.conf --server-data-dir=/tmp/rstudio-server-data",
}

Some things left to do:

  • enable no-auth (cannot figure it out, could use help!)
  • verify that it works with various versions of RStudio
  • open the browser automatically?
  • streamline install scripts further?

closes #71

Copy link
Member

@eitsupi eitsupi left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

However, for several reasons, I believe this needs a major rewrite.

  • Please do not copy scripts from https://github.com/rocker-org/rocker-versioned2. https://github.com/rocker-org/rocker-versioned2 is licensed under GPL-2.0 or later, so anything copied from it cannot be included in this repository under the MIT license.
  • It would be better not to depends on the apt-packages Feature. apt-packages is a very simple script and is unlikely to be updated, so I see no advantage in choosing to rely on it over copying it.
  • I don't think creating the default user here is a good idea. Shouldn't common-utils be used to create the user?
  • README.md is automatically generated by the CI, so please don't write by hand.

I have opened a new PR #174.
I will look at this in the next few days.

@rokroskar
Copy link
Contributor Author

Hi @eitsupi,

* Please do not copy scripts from https://github.com/rocker-org/rocker-versioned2. https://github.com/rocker-org/rocker-versioned2 is licensed under GPL-2.0 or later, so anything copied from it cannot be included in this repository under the MIT license.

Ouch I didn't notice the different license, thanks for pointing it out.

* It would be better not to depends on the `apt-packages` Feature. `apt-packages` is a very simple script and is unlikely to be updated, so I see no advantage in choosing to rely on it over copying it.

You mean instead of running apt directly in the install script? For basic dependencies it's nicer this way because it creates a separate layer in the resulting docker image so on repeated builds you benefit from caching.

* I don't think creating the default user here is a good idea. Shouldn't `common-utils` be used to create the user?

That's a great point - I didn't realize common-utils had that option. That should simplify a few things.

* `README.md` is automatically generated by the CI, so please don't write by hand.

I know, I wasn't sure if the automation assumed a README was there so I just added a very simple one.

In any case, given that you've opened up #174 I think we can close this...

@rokroskar rokroskar closed this Aug 9, 2023
@eitsupi
Copy link
Member

eitsupi commented Aug 10, 2023

For basic dependencies it's nicer this way because it creates a separate layer in the resulting docker image so on repeated builds you benefit from caching.

When do you mean repeat builds?

@rokroskar
Copy link
Contributor Author

Each feature goes into its own layer. So if I am making a change to the devcontainer.json that needs to rebuild some parts or that uses a new version of a feature, it doesn't need to build all the layers. In this case, if the apt dependencies are factored out and used as a feature dependency, they will be in their own layer and can be forced to install first so they are at the top of the resulting Dockerfile. This is nice for caching both locally (i.e. when using VSCode on my machine) and remotely (when using devcontainer tooling for building images in CI).

@eitsupi
Copy link
Member

eitsupi commented Aug 10, 2023

My understanding is that if no edits are made on devcontainer.json the layers of each Feature are not changed, for example if you change the options for another Feature, the rstudio layer is cached.
In other words, as I understand it, if you change the devcontainer.json file, except for changing the rstudio Feature options, the behavior of the two is the same and there is no advantage to relying on apt-packages.

@rokroskar
Copy link
Contributor Author

It depends - if I don't provide a full version, e.g. ghcr.io/rocker-org/rstudio:1 or ghcr.io/rocker-org/rstudio:latest then the devcontainer tooling will pull the latest version of the feature matching the spec, which result in a new layer needing to be built. You might argue that this is a trivial optimization in terms of layer caching. However, given that the apt step is basically needed in every single feature installation, it makes sense to me to factor this part out and reuse the code across the different features.

@eitsupi
Copy link
Member

eitsupi commented Aug 14, 2023

Thanks, that makes sense.

Given such benefits, I think devcontainers should provide a Feature to do so...... (devcontainers/features#537)

@rokroskar
Copy link
Contributor Author

Oh great, I wasn't aware of that, thank you for linking the issue.

@eitsupi
Copy link
Member

eitsupi commented Aug 14, 2023

In fact, this (devcontainers/features#67) is where the apt-packages concept comes from.

Since there was no indication that they would implement it I tried here.

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.

New feature: RStudio Server
2 participants