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 dx-specific user setup, install devcontainers/docker vscode extensions by default #745

Merged
merged 7 commits into from
Jan 1, 2024

Conversation

jeefy
Copy link
Contributor

@jeefy jeefy commented Dec 21, 2023

This sets up a fresh install of Bluefin to have VSCode set up to use a Distrobox container (via devcontainers)

There needs to be a shim in place to ensure you enter the distrobox container as the local user and not as root.

It does require Docker to be available on the host (which is a requirement of the devcontainers extension

Refs:
#726
https://universal-blue.discourse.group/t/should-we-really-recommend-vscode-podman-as-supported/220/13

@jeefy jeefy requested a review from castrojo as a code owner December 21, 2023 19:41
@bketelsen
Copy link
Member

Is there a place to document how to specify the distrobox container in the devcontainer configs?

@jeefy
Copy link
Contributor Author

jeefy commented Dec 21, 2023

Is there a place to document how to specify the distrobox container in the devcontainer configs?

There may be, but I don't think that's something we should set by default. Since the lifecycle of the Distrobox container is outside of VSCode, we shouldn't make assumptions on what the container is named.

Copy link
Member

@p5 p5 left a comment

Choose a reason for hiding this comment

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

This is great, although my only concern is this will try to install the VSCode extensions on all Bluefin images, whereas Bluefin DX is the only one where VSCode is installed.

Not sure what the best approach would be. I guess conditionally running the scripts based on the contents of IMAGE_INFO JSON file

@jeefy
Copy link
Contributor Author

jeefy commented Dec 22, 2023

I guess conditionally running the scripts based on the contents of IMAGE_INFO JSON file

Good call @p5, updated the PR with that :)

@castrojo
Copy link
Member

Any final comments on this one? I'd like to start testing this this weekend!

@m2Giles
Copy link
Member

m2Giles commented Dec 22, 2023

Would this break normal dev-containers forcing the use of distrobox due to the docker path being that shim?

auto-merge was automatically disabled December 31, 2023 18:06

Head branch was pushed to by a user without write access

@jeefy jeefy changed the title feat: add a default vscode config and install devcontainers extension feat: add dx-specific user setup, install devcontainers/docker vscode extensions by default Dec 31, 2023
@jeefy
Copy link
Contributor Author

jeefy commented Dec 31, 2023

After talking with @castrojo a bit, this is the route we settled on. :) Does change the PR quite a bit but it simplifies it. 🙌

@KyleGospo
Copy link
Member

Looks good, may want to increase the version at the top of the user setup to ensure this is run for everyone

@castrojo castrojo enabled auto-merge December 31, 2023 20:40
@castrojo
Copy link
Member

Will that clobber people's existing setups? I'm fine with it being new-install only so we don't interrupt people's existing flows.

@EyeCantCU
Copy link
Member

Will that clobber people's existing setups? I'm fine with it being new-install only so we don't interrupt people's existing flows.

Unfortunately that's true. If this was its separate thing (I think it should be a separate service since it uses an internet connection to install those extensions) it wouldn't be an issue though

@castrojo
Copy link
Member

Yeah can we do that @jeefy? Sorry to make this process longer than it needs to be. (On the plus side after this we're done!)

Copy link
Member

@EyeCantCU EyeCantCU left a comment

Choose a reason for hiding this comment

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

Left some comments regarding converting this over to a separate service and a couple other things

dx/usr/bin/bluefin-dx-user-setup Outdated Show resolved Hide resolved
usr/bin/ublue-user-setup Outdated Show resolved Hide resolved
usr/bin/ublue-user-setup Outdated Show resolved Hide resolved
auto-merge was automatically disabled December 31, 2023 21:45

Head branch was pushed to by a user without write access

@jeefy
Copy link
Contributor Author

jeefy commented Dec 31, 2023

All comments should be addressed.

If we add additional post-install config steps, we might want to figure out a better way to track what's run. A bunch of dot files in a home feelsbadman, but that's a problem for another PR. :)

@EyeCantCU
Copy link
Member

All comments should be addressed.

If we add additional post-install config steps, we might want to figure out a better way to track what's run. A bunch of dot files in a home feelsbadman, but that's a problem for another PR. :)

Things are looking fantastic! Definitely up for handling this in a better way. Having a lot of dotfiles is slightly annoying

@EyeCantCU
Copy link
Member

One final thing - the Containerfile needs the service to be enabled: systemctl enable --global bluefin-dx-user-vscode.service

@castrojo castrojo enabled auto-merge January 1, 2024 01:04
Copy link
Member

@EyeCantCU EyeCantCU left a comment

Choose a reason for hiding this comment

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

LGTM!

@castrojo castrojo disabled auto-merge January 1, 2024 01:07
@castrojo castrojo merged commit d6737cb into ublue-os:main Jan 1, 2024
34 checks passed
awesomekyle pushed a commit to awesomekyle/bluefin that referenced this pull request Apr 24, 2024
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.

7 participants