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

add devcontainer setup docs #154

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

tangowithfoxtrot
Copy link
Contributor

Objective

This adds documentation for VS Code Dev Containers, added by the associated PR in server.

@bitwarden-bot
Copy link

bitwarden-bot commented Jul 12, 2023

Logo
Checkmarx One – Scan Summary & Details5035d625-d5c4-4950-9182-d115a710098d

No New Or Fixed Issues Found

@tangowithfoxtrot tangowithfoxtrot marked this pull request as ready for review July 12, 2023 17:24
@tangowithfoxtrot tangowithfoxtrot requested a review from a team as a code owner July 12, 2023 17:24
@tangowithfoxtrot
Copy link
Contributor Author

tangowithfoxtrot commented Jul 12, 2023

I am not sure why the build workflow is having issues with the link used in the docs; the link works locally and is a direct copy from the standard "Setup Guide" steps.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 12, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: f3909e0
Status: ✅  Deploy successful!
Preview URL: https://a768163d.contributing-docs.pages.dev
Branch Preview URL: https://vscode-devcontainers.contributing-docs.pages.dev

View logs

Copy link
Contributor

@withinfocus withinfocus left a comment

Choose a reason for hiding this comment

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

Only jumped around for a moment and not a full review, but I think you have link path issues it can't resolve so the build's failing.

Once this renders and we can see a preview it'll be easier to read.

@@ -22,6 +22,13 @@ Before you start: make sure you’ve installed the recommended

:::

:::info

A guide for developing in VS Code Dev containers can be found [here](./guide-vscode) if you prefer
Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ Capital "C" for "Containers".

This is more of my style I suppose, but I try to future-proof this a bit with something like a list format. This could instead be reworded as a collection of guides available, and then you might want to have this in a subfolder of "guides". We have VS Code and VS material here and perhaps they should follow your pattern here so it isn't unique and on its own.

I realize that's easy to ask, but it'd be a nice structure change.

Changing `MSSQL_SA_PASSWORD` variable after first running the Dev Containers will require a
re-creation of the storage volume.

**Warning: this will delete your development database.**
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a warn decorator you can use here, but not sure if it works within a surrounding block.

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 updated the language and formatting for this process to hopefully make it more clear.

@withinfocus
Copy link
Contributor

@tangowithfoxtrot my other comment may have been missed -- link errors are blocking the build.

@tangowithfoxtrot
Copy link
Contributor Author

tangowithfoxtrot commented Jul 12, 2023

@withinfocus My bad. I did miss your comment there. I can look into an alternative link format, but I'm confused as to why the build workflow would claim that it's broken, as it's a direct copy from the existing "Setup Guide" doc and works with my local build.

@tangowithfoxtrot
Copy link
Contributor Author

as it's a direct copy from the existing "Setup Guide" doc and works with my local build.

Whoops. That was incorrect 😁 Looking into it now...

Copy link
Contributor

@withinfocus withinfocus left a comment

Choose a reason for hiding this comment

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

I think it'd be a good idea to pull the repeated content and see how we can thin this new documentation down.

I am definitely interested in using this!

you can run `dotnet format` from the command line when convenient (e.g. before requesting a PR
review).

## Configure Env File
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 Can we start this documentation right here? The prior two steps are a repeat of the main setup guide.

Copy link
Contributor

Choose a reason for hiding this comment

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

⛏️ Would be nice to use .env instead of "Env".

Comment on lines +76 to +89
:::caution

Your MSSQL password must comply with the following
[password complexity guidelines](https://docs.microsoft.com/en-us/sql/relational-databases/security/password-policy?view=sql-server-ver15#password-complexity)

- It must be at least eight characters long.
- It must contain characters from three of the following four categories:
- Latin uppercase letters (A through Z)
- Latin lowercase letters (a through z)
- Base 10 digits (0 through 9)
- Non-alphanumeric characters such as: exclamation point (!), dollar sign ($), number sign (#), or
percent (%).

:::
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Can we include this somewhere more general, or on the setup guide itself?

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 actually exists in the standard setup guide as well. I could remove this call-out entirely from the dev container, but I think I'd still run into the issue described here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, perhaps that call-out from both guides could be moved here?


4. You can change the other variables or use their default values. Save and quit this file.

### SQL Server
Copy link
Contributor

Choose a reason for hiding this comment

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

💭 As I read down here I am seeing more repeated content. How can we better push this all upward and just focus on the Dev Containers content? Keeps it DRY.

Copy link
Contributor Author

@tangowithfoxtrot tangowithfoxtrot Jul 13, 2023

Choose a reason for hiding this comment

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

I started this guide as a copy of the server setup guide and took the approach of removing mention of setup steps that the containers handle for us now. There's definitely still some repeating content that I left in, as there are a few things that couldn't be fully automated without a ton of workarounds (mostly for Windows hosts) that still require some user interaction.

I could try to link back to the standard server setup guide for some of the repeating sections, but many of them have just one or two steps that vary just based on the fact that the parts of the setup that require manual interaction (env and secrets) is largely the same, but the way you have to interact with things like that and starting the DB, Azurite, and other containers varies slightly.

As an example, the step that follows step #\4 from the above section in the standard setup guide instructs the user to run docker compose up ... to start the container stack, but doing so using the dev container approach would be redundant, and possibly confusing. I'm unsure of how I might utilize existing parts of the standard setup guide for things like this.

I'd definitely appreciate your thoughts though. I'd love to simplify the docs for this as much as possible and reuse what we can.

Copy link
Contributor Author

@tangowithfoxtrot tangowithfoxtrot Jul 13, 2023

Choose a reason for hiding this comment

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

The first thought that comes to mind if we wanted to have slight variance in the steps that do still manually require some user interaction would be to add a "Dev Container" option to the top-right dropdown menu, or something like that.

Doing so might also be weird though because that menu currently distinguishes between internal engineering and external community dev configuration guides. The dev containers also have their own community/internal configurations, so we'd have to further subdivide that in a way that makes sense.

@@ -1,2 +1,2 @@
label: "Databases"
position: 2
position: 3
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ As I read through all this I started to feel like the guide in this PR is a sub-page of the server's getting started page since there's repeated content. I think this needs some organizing and you wouldn't need this move.

Copy link
Member

@Hinton Hinton Jul 13, 2023

Choose a reason for hiding this comment

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

I agree, maintaining the cloud and self-hosted configs are already confusing enough without adding a third document. It would be nice to have a single guide that gives you two options towards the end.

Copy link
Contributor Author

@tangowithfoxtrot tangowithfoxtrot Jul 13, 2023

Choose a reason for hiding this comment

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

I'd love to integrate the container setup into the standard setup guide, but I am not sure how to do so, as even the sections that are shared between the two setups have a few steps that vary slightly.

Is there some way to indicate variance of the setup process at the step level without adding new "Dev Container - Bitwarden Dev" "Dev Container - Community Dev" options in the dropdown menu?

Copy link
Member

@Hinton Hinton Jul 14, 2023

Choose a reason for hiding this comment

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

Maybe tabs? Docusaurus mdx lets you write & use react components if you want. And we could even look into switching the getting started page into a react page if needed.

@withinfocus
Copy link
Contributor

With bitwarden/server#3080 landed should we look at this again?

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.

4 participants