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

Create devcontainer configuration #6479

Merged
merged 8 commits into from
Sep 7, 2023

Conversation

spenserblack
Copy link
Contributor

Description

See #6473

Currently, prebuilds are available from my fork, built from the devcontainer branch. Hopefully none of the reviewers of this PR are in East Asia, because that's the one region I didn't enable in order to save on storage usage 🙃

This will probably need some further changes. Some things I can think of off the top of my head are

  • Do we want to use a specific Ruby version, or just stick with the latest?
  • Any additional desired VS Code extensions or settings?
  • Right now script/bootstrap is a post-create command, and it can apparently hit 100% CPU usage with a 2-core codespace. Should the min requirements be specified as 4-core?

Checklist:

(Don't think any of the checkboxes apply, sorry if one does)

This is to install additional dependencies that don't come with base
image.
@spenserblack spenserblack requested a review from a team as a code owner July 15, 2023 16:44
@lildude
Copy link
Member

lildude commented Jul 21, 2023

This is looking good. I'll see if I can find time to play in the next week or two.

One thing we'll definitely want to do as part of this PR is update the CONTRIBUTING.md file to detail how peeps can use this for Codespaces and local docker dev.

Do we want to use a specific Ruby version, or just stick with the latest?

Latest released version to align more closely with GitHub production.

Any additional desired VS Code extensions or settings?

TBC. I'll need to check my original branch as I know I added some.

Right now script/bootstrap is a post-create command, and it can apparently hit 100% CPU usage with a 2-core codespace. Should the min requirements be specified as 4-core?

I'm undecided on this ATM. Yes, because it's a better experience and doesn't result in the warning message. No, because it does work and is only an issue during the initial cloning of the grammars, it shouldn't be an issue with prebuilds (might need a few tweaks), and because it costs users more. I'll think about it.

@spenserblack
Copy link
Contributor Author

spenserblack commented Jul 26, 2023

Oh, just found out that some tests will error out in the codespace because some refs used for testing don't exist.
Looks like copying this would fix it:

https://github.com/github-linguist/linguist/blob/e855ef2b6f90c34074061a2e17acbe853e61b483/.github/workflows/ci.yml#L32C9-L32C109

Since it should work for both the main repo and forks, and forks by default only copy the main branch, perhaps this could be part of the post create command:

git remote add linguist https://github.com/github-linguist/linguist
git fetch linguist ...

Or we could get more advanced and detect if upstream exists, if so fetch from that, else fetch from origin.

@spenserblack spenserblack marked this pull request as draft August 21, 2023 15:46
These references are necessary for tests to work properly.
@spenserblack spenserblack marked this pull request as ready for review August 21, 2023 16:57
Most peeps will be editing the YAML files so lets ensure a better experience
This will allow the bootstrapping to be cached in the prebuild and save users a lot of time
@lildude
Copy link
Member

lildude commented Sep 7, 2023

I've had a chance to play with this today and it's looking good. I've made a few tweaks:

  • I've added the edhat.vscode-yaml extension as peeps will mostly be editing the YAML files so lets help make it a better experience
  • I've moved the bootstrapping to onCreateCommand so it can be cached as part of prebuilding

Can you please also test this and know what you think @spenserblack

Copy link
Collaborator

@Alhadis Alhadis left a comment

Choose a reason for hiding this comment

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

Minor nitpick. Don't consider it a blocker or anything.

.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
.devcontainer/devcontainer.json Outdated Show resolved Hide resolved
Co-authored-by: John Gardner <gardnerjohng@gmail.com>
@spenserblack
Copy link
Contributor Author

Thanks @lildude and @Alhadis!

Can you please also test this and know what you think @spenserblack

Just tested quickly by letting a prebuild run, opening a codespace, and running bundle exec rake test. Works like a charm!

Unfortunately, I'm at >90% of my codespace storage quota, so after that quick test I promptly deleted my codespace and the prebuilds 😅

Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

Thanks for this @spenserblack 🙇

@lildude lildude merged commit 826cc7d into github-linguist:master Sep 7, 2023
5 checks passed
@spenserblack spenserblack deleted the devcontainer branch September 7, 2023 14:06
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants