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

Dev container configurations into dev branch #8

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from

Conversation

robigan
Copy link
Contributor

@robigan robigan commented Aug 29, 2022

Add certain configurations that allow for the repository to be seamlessly opened in a local dev container in VSCode, Docker Dev Containers, or GitHub Workspaces. Also add configuration for VSCode Monorepo workspace support

Copy link
Member

@ffraenz ffraenz left a comment

Choose a reason for hiding this comment

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

Thank you for your pull request! I've left some comments to your changes. Also, we need to update the README to explain how to use the dev container.

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/ciphereditor.code-workspace Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Show resolved Hide resolved
.devcontainer/ciphereditor.code-workspace Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Show resolved Hide resolved
Copy link
Contributor Author

@robigan robigan left a comment

Choose a reason for hiding this comment

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

Changes will be applied soon

@robigan robigan marked this pull request as draft August 29, 2022 18:31
@robigan
Copy link
Contributor Author

robigan commented Aug 29, 2022

Gonna take a shower and then fix the changes

@vercel
Copy link

vercel bot commented Aug 29, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
ciphereditor ✅ Ready (Inspect) Visit Preview Aug 31, 2022 at 8:32PM (UTC)

@robigan robigan marked this pull request as ready for review August 29, 2022 19:19
@robigan robigan requested a review from ffraenz August 29, 2022 19:21
Copy link
Member

@ffraenz ffraenz left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you! How should we let people know about all of this in the README? Should we add a badge like in https://github.com/microsoft/vscode-remote-try-node?

.devcontainer/devcontainer.json Show resolved Hide resolved
.devcontainer/devcontainer.json Show resolved Hide resolved
@robigan
Copy link
Contributor Author

robigan commented Aug 30, 2022

Awesome! Thank you! How should we let people know about all of this in the README? Should we add a badge like in https://github.com/microsoft/vscode-remote-try-node?

If you want, I can update the README with these setup steps and add the VSCode Remote Containers badge

@ffraenz
Copy link
Member

ffraenz commented Aug 30, 2022

Yes, please! It would round off this pull request. I'd keep the dev container setup as an alternative to the current setup, though. Let's divide the development setup section into two.

@robigan
Copy link
Contributor Author

robigan commented Aug 30, 2022

Ok, I finally managed to fix my docker setup after realizing that I had to install some kernel modules and then forgetting to restart cus of new kernel. But alright let me do that rq

Adds configs for dev containers for quick spin up of dev environment
Add .gitkeep to assets folder such that dev setup docs are valid
Add .code-workspace file to .gitignore for users to modify without issue
@ffraenz
Copy link
Member

ffraenz commented Aug 30, 2022

@robigan I've committed my suggestion for the README into the PR. What do you think?

@ffraenz
Copy link
Member

ffraenz commented Aug 30, 2022

After further thought I'm making the conventional setup the primary option here as it does not require the use of Docker or Visual Studio Code. Still, I want to roll with dev containers and see where it leads us. I hope you can relate.

@robigan
Copy link
Contributor Author

robigan commented Aug 31, 2022

I totally agree, that is why I left the conventional setup at the top of the setup instructions, as those are the first instructions the human eye will see. If you're willing to mark it as primary to make it easier, then please go ahead

@robigan
Copy link
Contributor Author

robigan commented Aug 31, 2022

@robigan I've committed my suggestion for the README into the PR. What do you think?

TBH you did a much better job than I did, your changes on the README or way cleaner than mine hahaha

Comment on lines +22 to +24

# Workspaces
/.devcontainer/ciphereditor.code-workspace
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed, you are gitignoring a file that is included in your PR. Which one of the two were you aiming for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be like that, but if you want I'll remove that, as the monorepo extension modifies it, but it should be included in the repo, just not tracked for changes

Copy link
Member

@ffraenz ffraenz Aug 31, 2022

Choose a reason for hiding this comment

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

As soon as a file gets tracked (which is the case for ciphereditor.code-workspace as it is visible here), .gitignore does no longer affect it. So the file should either be included and not ignored or vice-versa. Or am I missing something?

The Visual Studio Code workspace file does change along the way when working with it, right? Maybe it should not be part of version control?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea that's what I was hoping to do, I recall having done something similar long ago but I think am getting confused. Do you think it might be worth adding a separate template file for tracking, and add a copy command to the dev container postSetup to copy from template to the normal workspace file?

Copy link
Member

Choose a reason for hiding this comment

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

I just found that the vscode settings could be moved to devcontainer.json. I'd suggest you to do that and remove the workspace file entirely from the project and .gitignore.

Copy link
Member

Choose a reason for hiding this comment

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

What is wrong with adding the vscode settings in there? Am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Conventions reasons. As configurations for VSCode pertaining to if the Dev Container is used should be put in the dev container. Settings pertaining to VSCode of VSC Workspaces are used (regardless of usage of Dev Containers) should be in a .code-workspace file. General project settings regardless of the usage of Dev Containers and/or VSC workspace. In this case, since the Monorepo plugin could initiate/generate the workspace configuration, then it's settings should be in the settings.json file in .vscode folder, indeed it was stupid of me originally to embed them into the workspace file, as it was assuming that dev containers were using and the workspace configuration was activated before hand

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that VSC Workspaces and Dev Containers are separate things, In my experience, both in my personal projects and other public projects I have seen VSC Workspace configurations be included alongside Dev Container configurations, as it's easier for a monorepo developer to receive bug reports and PRs that use the same highly reproducible environments.

Copy link
Member

Choose a reason for hiding this comment

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

The reason I am restrained here: We're not sponsored by Microsoft and I try to keep the repository as neutral as possible when it comes to the choice of editor people want to use. I'd like to take dev containers to a test run but I don't want to make vscode the 'preferred way' of doing things. I'd rather go for stuff like https://editorconfig.org/, etc. So what I'm really asking is, what is a sensible minimum of vscode specific stuff we can put in to make dev containers work nicely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, I am more than happy to write and maintain other configurations for other editors that I use too if need be, that is the beauty of Open source. It's true that I may have a bias towards this PR, but from my experience, I don't believe that writing extra configurations for a certain editor really is restraining the repo on which editor is best to use, at the end of the day, anyone can write and contribute configurations for their editors, this is how the bigger repos work with their community base of contributors.

I took a look at editorconfig, it's a pretty cool idea, but its scope is already handled by ts-standard and the underlying ESLint.

@ffraenz ffraenz added the feature New feature or request label Aug 31, 2022
Updated Dev Container configuration in next commit
- More friendly UI
- Included various checks
- Included workspace config template copying
@robigan robigan marked this pull request as ready for review August 31, 2022 20:32
@robigan robigan requested a review from ffraenz August 31, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants