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

build: add devcontainer configuration #40825

Closed
wants to merge 6 commits into from
Closed

Conversation

bnb
Copy link
Contributor

@bnb bnb commented Nov 15, 2021

this PR adds two metadata files:

  1. ./.devcontainer/.devcontainer.json provides metadata to GitHub Codespaces and other tools (like Docker Desktop's preview Developer Environments feature) about how to build and configure a developer environment.
  2. ./.devcontainer/Dockerfile a very simple Dockerfile that the .devcontainer.json file uses to build from. This will consume the Docker image I've built out (pending move bnb/devenv to nodejs/devcontainer admin#641 on moving it into nodejs/devcontainer) that will be built and published nightly.

These should add a relatively low maintenance burden - the Dockerfile is literally two LOC (most of the work for this is done in the nodejs/devcontainer repo!) and the .devcontainer.json is effectively a configuration file that represents some settings. It could be stripped down or more thoroughly built out if folks want, though I'd generally like to request that we err on configuring for a broader audience than just project members (think the audience at a Code and Learn event) while also serving ourselves.

This PR should not land until nodejs/admin#641 lands.

Signed-off-by: Tierney Cyren <hello@bnb.im>
Signed-off-by: Tierney Cyren <hello@bnb.im>
@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Nov 15, 2021
@bnb
Copy link
Contributor Author

bnb commented Nov 15, 2021

I should also add:

The intent of adding these files is to allow for easier direct development of Node.js. It means that when you try to launch Codespaces from nodejs/node in GitHub, you'd be set up with the environment defined in these configuration files rather than a raw default environment with a git clone of the source and nothing else. It also means that you'd be able to effectively do the same with Docker Desktop's Development Environment preview and any future implementors of the devcontainer config format (GitPod, for example, has this as a roadmap item).

These configurations are not required to use the Docker image from nodejs/admin#641 as a developer environment. Rather, they take the image built from that repo and extend it slightly to improve the experience outside of those who want to figure out how to launch the Docker container that repo builds locally or how to spin it up as a chonky VM in the cloud and SSH in to it.

}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The indentation in this file is inconsistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed ty

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually fixed now :P

Signed-off-by: Tierney Cyren <hello@bnb.im>
.devcontainer/Dockerfile Outdated Show resolved Hide resolved
@F3n67u
Copy link
Member

F3n67u commented May 25, 2022

is this pr still relevant?

I am using GitHub codespace for node source code development, but we have no devcontainer configuration to config the docker env of GitHub codespace so I have to install ninja manually.

@bnb
Copy link
Contributor Author

bnb commented May 26, 2022

This PR is still relevant (imo). It was stalled for a long time waiting on us to get a Docker org. We now have that. Unfortunately around that time I was feeling pretty drained and was beginning to feel the need to job search. I've now job searched, quit my job, and am on a brief mental health recovery period that was graciously agreed to by my new employer before working again.

This is maybe like 5 hours of work from being complete, not including figuring out who in the org owns the Node.js Docker Hub org and setting that governance up.

Codespaces does work now, but kinda sucks as you mention. The Docker image has been transferred into the org but isn't being built/deployed presently. I'll be able to spend some time on this in the back half of June! Also happy to pair on it at Summit if anyone would like that.

Signed-off-by: Tierney Cyren <hello@bnb.im>
@bnb
Copy link
Contributor Author

bnb commented Jun 6, 2022

some small updates from airplane driven development:

  • re-tested the Docker image to make sure it still works (it does!)
  • updated the nodejs/devcontainer repo with some more documentation from my exploration of re-testing it with fresh eyes/a fresh computer (chore: doc and automation updates devcontainer#3)
  • updated the tag in nodejs/devcontainer to be nightly not latest, since it's more accurate (chore: doc and automation updates devcontainer#3)
  • updated this PR to address @targos's feedback
  • tweaked the Docker image tag from latest to nightly since that's both what's in nodejs/devcontainer and a closer match to what we're actually initially doing with that repo.

@bnb
Copy link
Contributor Author

bnb commented Jun 6, 2022

tl;dr update on this:

  • Code in the repo (nodejs/devcontainer) seems to work
  • This addition is minimal but relies on that code
  • The remaining hurdle is figuring out how to set up auth and publishing to our org (nodejs) on DockerHub, since that's a new property for us.

I'd love to try to get that last part done while in-person at OpenJS World in Austin. It should be extremely do-able. If we can unblock that, we should be good to get the nodejs/devcontainer repo automated, let it simmer for a bit and see if there are any problems, and then from there merge this PR.

@bnb
Copy link
Contributor Author

bnb commented Jun 11, 2022

Tagging with TSC agenda per a discussion I had with a TSC member:

Currently, the only thing blocking this from landing (outside of the failures that shouldn't be failing?) is working with someone to get the Docker auth setup. The OpenJS Foundation staff have the keys to the org that we need, so the remaining task is just doing that manual work to get things set up.

I'm happy to pair on this with anyone almost anytime in the next ~month since I'll have plenty of time. Just need to figure out who/when :)

@mcollina
Copy link
Member

With Brian leaving OpenJS Foundation I do not know who to ask, but we can ask Robin to direct us.

@mcollina
Copy link
Member

@nodejs/build are there any volunteers that would like to pair with Tierney?

@ErickWendel
Copy link
Member

@nodejs/build are there any volunteers that would like to pair with Tierney?

✋🏻

@mhdawson mhdawson removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jul 6, 2022
@mhdawson
Copy link
Member

mhdawson commented Jul 6, 2022

Removing from the TSC agenda, Tierney walked me through the setup.

@bnb
Copy link
Contributor Author

bnb commented Jul 6, 2022

small status update: the repo (nodejs/devcontainer) seems to be working. Before taking her leave, Jory got me the necessary credentials to Make It Work (we should discuss making it more under our control at a later point - currently publishing from the Foundation account to the nodejs org on Docker Hub). It published the first container successfully. I want to let it publish nightly for at least a week without problems before considering requesting this PR be merged, but we shouldn't be far off of that now 👍🏻

@bnb
Copy link
Contributor Author

bnb commented Jul 28, 2022

Just a small update: The Docker image seems to have successfully built + published for the past 23 days! 🎉

I just followed the instructions and it also seems to be working exactly as one would hope (everyone who's going to use this might want to start working on a custom environment setup script to make it pretty how you like it):

image

On this PR: I need to figure out what about the setup is currently killing all actions. Once I've done that, we should be good to go.

@richardlau
Copy link
Member

On this PR: I need to figure out what about the setup is currently killing all actions. Once I've done that, we should be good to go.

4d90714 added a Git submodule (devcontainer) and the action workflows are complaining about that.

@bnb
Copy link
Contributor Author

bnb commented Jul 28, 2022

4d90714 added a Git submodule (devcontainer) and the action workflows are complaining about that.

Gotcha, I wouldn't have figured that out fast so thank you. I've literally never touched submodules so must have somehow done this accidentally and will figure out how to undo it 😅

Signed-off-by: Tierney Cyren <hello@bnb.im>
@bnb
Copy link
Contributor Author

bnb commented Oct 10, 2022

Force pushed the same changes hopefully sans submodule 🤞🏻

Signed-off-by: Tierney Cyren <hello@bnb.im>
@bnb
Copy link
Contributor Author

bnb commented Oct 10, 2022

If tests pass, this should be mergable. Since this theoretically doesn't change any of Node.js itself, tests should pass 👀

@Trott
Copy link
Member

Trott commented Apr 21, 2023

Putting this in draft mode and then putting it back in ready-for-review so that the tests can re-run. Current tests show failures that are almost certainly unrelated.

@Trott Trott marked this pull request as draft April 21, 2023 21:44
@Trott Trott marked this pull request as ready for review April 21, 2023 21:44
@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 22, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 22, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/40825
✔  Done loading data for nodejs/node/pull/40825
----------------------------------- PR info ------------------------------------
Title      build: add devcontainer configuration (#40825)
Author     Tierney Cyren  (@bnb)
Branch     bnb:bnb/add-devcontainer -> nodejs:main
Labels     meta
Commits    6
 - build: don't ignore devcontainer
 - build: add devcontainer configs
 - build: fix inconsistent spacing
 - chore: remove unnessary WORKDIR
 - build: tweaks to various devcontainer configs
 - build: remove erroneous devcontainer submodule
Committers 1
 - Tierney Cyren 
PR-URL: https://github.com/nodejs/node/pull/40825
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/40825
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - build: tweaks to various devcontainer configs
   ⚠  - build: remove erroneous devcontainer submodule
   ℹ  This PR was created on Mon, 15 Nov 2021 23:50:30 GMT
   ✔  Approvals: 1
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/40825#pullrequestreview-806724188
   ✘  Last GitHub CI failed
   ℹ  Green GitHub CI is sufficient
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/4773575905

Trott pushed a commit that referenced this pull request Apr 22, 2023
PR-URL: #40825
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Apr 22, 2023

Landed in 0c5f253

@Trott Trott closed this Apr 22, 2023
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
PR-URL: nodejs#40825
Reviewed-By: James M Snell <jasnell@gmail.com>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 28, 2023
PR-URL: nodejs#40825
Reviewed-By: James M Snell <jasnell@gmail.com>
yjl9903 pushed a commit to yjl9903/node that referenced this pull request Apr 29, 2023
PR-URL: nodejs#40825
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request May 2, 2023
PR-URL: #40825
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request May 2, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
PR-URL: #40825
Reviewed-By: James M Snell <jasnell@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
PR-URL: nodejs#40825
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-failed An error occurred while landing this pull request using GitHub Actions. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.