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

Vscode devcontainers #3080

Merged
merged 23 commits into from
Nov 6, 2023
Merged

Vscode devcontainers #3080

merged 23 commits into from
Nov 6, 2023

Conversation

tangowithfoxtrot
Copy link
Contributor

@tangowithfoxtrot tangowithfoxtrot commented Jul 9, 2023

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

Objective

This adds VSCode devcontainers, which should make the dev environment setup process easier for those that choose to utilize this workflow.

Code changes

  • .devcontainer/bitwarden_common/docker-compose.yml: a baseline set of services that are used in both the internal and community dev configurations

  • .devcontainer/(community_dev|internal_dev)/devcontainer.json: the declaration files for the VSCode dev containers

  • .devcontainer/internal_dev/docker-compose.override.yml: adds Azurite container for internal dev config

  • .devcontainer/(community_dev|internal_dev)/postCreateCommand.sh: these optional one-time setup scripts configure secrets.json with the DB password, installation ID and key, and certificate thumbprints to enable running setup_secrets.ps1 automatically after the devcontainer initializes

Before you submit

  • Please check for formatting errors (dotnet format --verify-no-changes) (required)
  • If making database changes - make sure you also update Entity Framework queries and/or migrations
  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team

@bitwarden-bot
Copy link

bitwarden-bot commented Jul 9, 2023

Logo
Checkmarx One – Scan Summary & Details3695b91e-73ec-4b38-8d60-69df843fa72b

New Issues

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Healthcheck Not Set /docker-compose.yml: 26 Check containers periodically to see if they are running properly.
MEDIUM Healthcheck Not Set /docker-compose.yml: 4 Check containers periodically to see if they are running properly.
MEDIUM Healthcheck Not Set /docker-compose.override.yml: 4 Check containers periodically to see if they are running properly.
MEDIUM Healthcheck Not Set /docker-compose.yml: 11 Check containers periodically to see if they are running properly.
MEDIUM Host Namespace is Shared /docker-compose.yml: 4 The hosts process namespace should not be shared by containers
MEDIUM Host Namespace is Shared /docker-compose.yml: 26 The hosts process namespace should not be shared by containers
MEDIUM Host Namespace is Shared /docker-compose.override.yml: 4 The hosts process namespace should not be shared by containers
MEDIUM Host Namespace is Shared /docker-compose.yml: 11 The hosts process namespace should not be shared by containers
MEDIUM Memory Not Limited /docker-compose.override.yml: 4 Memory limits should be defined for each container. This prevents potential resource exhaustion by ensuring that containers consume not more than t...
MEDIUM Memory Not Limited /docker-compose.yml: 4 Memory limits should be defined for each container. This prevents potential resource exhaustion by ensuring that containers consume not more than t...
MEDIUM Memory Not Limited /docker-compose.yml: 11 Memory limits should be defined for each container. This prevents potential resource exhaustion by ensuring that containers consume not more than t...
MEDIUM Memory Not Limited /docker-compose.yml: 26 Memory limits should be defined for each container. This prevents potential resource exhaustion by ensuring that containers consume not more than t...
MEDIUM Networks Not Set /docker-compose.yml: 11 Setting networks in services ensures you are not using dockers default bridge (docker0), which shares traffic bewteen all containers.
MEDIUM Networks Not Set /docker-compose.yml: 4 Setting networks in services ensures you are not using dockers default bridge (docker0), which shares traffic bewteen all containers.
MEDIUM Networks Not Set /docker-compose.override.yml: 4 Setting networks in services ensures you are not using dockers default bridge (docker0), which shares traffic bewteen all containers.
MEDIUM Networks Not Set /docker-compose.yml: 26 Setting networks in services ensures you are not using dockers default bridge (docker0), which shares traffic bewteen all containers.
MEDIUM Security Opt Not Set /docker-compose.yml: 11 Attribute 'security_opt' should be defined.
MEDIUM Security Opt Not Set /docker-compose.yml: 26 Attribute 'security_opt' should be defined.
MEDIUM Security Opt Not Set /docker-compose.override.yml: 4 Attribute 'security_opt' should be defined.
MEDIUM Security Opt Not Set /docker-compose.yml: 4 Attribute 'security_opt' should be defined.
LOW Container Capabilities Unrestricted /docker-compose.override.yml: 4 Some capabilities are not needed in certain (or any) containers. Make sure that you only add capabilities that your container needs. Drop unnecessa...
LOW Container Capabilities Unrestricted /docker-compose.yml: 11 Some capabilities are not needed in certain (or any) containers. Make sure that you only add capabilities that your container needs. Drop unnecessa...
LOW Container Capabilities Unrestricted /docker-compose.yml: 4 Some capabilities are not needed in certain (or any) containers. Make sure that you only add capabilities that your container needs. Drop unnecessa...
LOW Container Capabilities Unrestricted /docker-compose.yml: 26 Some capabilities are not needed in certain (or any) containers. Make sure that you only add capabilities that your container needs. Drop unnecessa...
LOW Cpus Not Limited /docker-compose.yml: 11 CPU limits should be set because if the system has CPU time free, a container is guaranteed to be allocated as much CPU as it requests
LOW Cpus Not Limited /docker-compose.override.yml: 4 CPU limits should be set because if the system has CPU time free, a container is guaranteed to be allocated as much CPU as it requests
LOW Cpus Not Limited /docker-compose.yml: 26 CPU limits should be set because if the system has CPU time free, a container is guaranteed to be allocated as much CPU as it requests
LOW Cpus Not Limited /docker-compose.yml: 4 CPU limits should be set because if the system has CPU time free, a container is guaranteed to be allocated as much CPU as it requests

Copy link
Member

@justindbaur justindbaur left a comment

Choose a reason for hiding this comment

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

I'm so sorry for sitting on this to long, I still want to get this in and this will make things so much easier.

"vscode": {
"settings": {},
"features": {},
"extensions": ["ms-dotnettools.csharp"]
Copy link
Member

Choose a reason for hiding this comment

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

Since I sat on this to long the C# Dev Kit has come out and we should probably use that instead.

Suggested change
"extensions": ["ms-dotnettools.csharp"]
"extensions": ["ms-dotnettools.csdevkit"]

justindbaur
justindbaur previously approved these changes Oct 27, 2023
Copy link
Member

@justindbaur justindbaur 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 so much, @MGibson1 you're up!

@eliykat eliykat requested a review from MGibson1 October 30, 2023 04:53
Copy link
Member

@MGibson1 MGibson1 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 I only have two comments, but this feature is totally new to me

  1. I might be old fashioned, but I like manually controlling when migrations are run in dev
  2. ⚠️ the correct sdk does not appear to be installed by the containers by default. Is it possible to add this to the post-install script?

@tangowithfoxtrot
Copy link
Contributor Author

@MGibson1

  1. The reason I initially chose to utilize the Admin project to run the migrations automatically was because the migration.ps1 script relies on a docker container to run them, which would require setting up Docker-in-Docker and/or mounting the host's docker.sock, which is very hacky, imo. However, @justindbaur informed me about ./util/MsSqlMigratorUtility, so I've updated the containers to use that instead (in the first-time setup script) and reverted the automatic migrations change in Admin. We can now run manual migrations from within the devcontainer by running dotnet run --project ./util/MsSqlMigratorUtility "$SQL_CONN_STRING" instead of the migrate.ps1 script.

  2. I've adjusted the devcontainer image to use mcr.microsoft.com/devcontainers/dotnet:dev-6.0. Previously, it used mcr.microsoft.com/devcontainers/dotnet:0-6.0, which, as you mentioned, has an older SDK version. The dev image uses 6.0.416. Unfortunately, I can't get it to perfectly match 6.0.415, as declared in global.json because Microsoft hasn't published that exact version for the devcontainer image.


echo "Running migrations..."
sleep 5 # wait for DB container to start
dotnet run --project ./util/MsSqlMigratorUtility "$SQL_CONNECTION_STRING"
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 -- this is how our cloud is now performing migrations.

@withinfocus
Copy link
Contributor

I like where this is at but will let the other two guys give an approval. Warning: auto-merge is on.

@tangowithfoxtrot tangowithfoxtrot merged commit 66c5ccf into master Nov 6, 2023
@tangowithfoxtrot tangowithfoxtrot deleted the vscode-devcontainers branch November 6, 2023 21:58
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.

5 participants