Skip to content

Cleanup and annotate devcontainer.json#58

Closed
craiglpeters wants to merge 3 commits intogithub:mainfrom
craiglpeters:main
Closed

Cleanup and annotate devcontainer.json#58
craiglpeters wants to merge 3 commits intogithub:mainfrom
craiglpeters:main

Conversation

@craiglpeters
Copy link

Removed empty commands, and the nonsensical waitFor.

Annotated for readability

Copy link

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Left few minor comment, besides that looks good to me! ⚡


// Upon Codespaces or image prebuild, refresh the npm and node.hs installation (async from environment creation)
"updateContentCommand": "npm install",
"postCreateCommand": "",

Choose a reason for hiding this comment

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

I wonder if this was explicitly added to avoid the oryx tool from running? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup it was 👍

Copy link
Author

Choose a reason for hiding this comment

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

That is a dirty hack! No other way to do that?

"hostRequirements": {
"cpus": 4
},
"waitFor": "onCreateCommand",
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that using this instead of the default updateContentCommand means we can connect sooner, while we're installing dependencies, as opposed to waiting for dependencies to be installed.

That's mostly a moot point, given that we're using prebuilds but still seems valuable?

Copy link
Author

Choose a reason for hiding this comment

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

I don't see how it is valuable if we're using Codespaces prebuilds. Help me understand

Copy link
Contributor

Choose a reason for hiding this comment

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

Two thoughts:

  1. If prebuilds are broken or stale, it helps
  2. If folks are copying & pasting the devcontainer from these templates to use in their own projects, this is a reasonable default

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense. What would you provide as a comment inline to explain the wait for an empty command?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about: "Allow connecting earlier, while we're still installing dependencies"

Craig Peters and others added 2 commits November 1, 2023 11:13
Co-authored-by: Samruddhi Khandale <samruddhikhandale@github.com>
Co-authored-by: Samruddhi Khandale <samruddhikhandale@github.com>
@@ -1,21 +1,34 @@
{

Choose a reason for hiding this comment

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

.devcontainer/devcontainer.json

@@ -1,21 +1,34 @@
{
Copy link

Choose a reason for hiding this comment

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

// See the devcontainer spec at https://containers.dev/implementors/spec/
"name": "React Codespaces Template",

// Container image to build environment from
// universal contains many tools making it useful in many contexts https://github.com/devcontainers/images/tree/main/src/universal
"image": "mcr.microsoft.com/devcontainers/universal:2",

// Specify the minimum machine spec for running the dev container
"hostRequirements": {
"cpus": 4
},

Copy link

Choose a reason for hiding this comment

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

Ya just got ripped big

@jaibandhu
Copy link

ok

@joshaber joshaber closed this May 24, 2025
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.

10 participants