Add initial config for devcontainers. #689
Conversation
📝 WalkthroughWalkthroughThis update introduces a development container configuration file that sets up an environment with Node.js, TypeScript, and Yarn using a specified Docker image. It also defines a post-creation command to install dependencies via Yarn. In addition, the contribution guidelines have been updated with a new section on using development containers, providing instructions for installing necessary tools and reopening the repository in a container environment, along with minor formatting adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant D as Developer
participant V as VS Code Dev Container
participant DC as Docker Container
D->>V: Open project in container mode
V->>DC: Build container using .devcontainer config
DC->>DC: Execute postCreateCommand ("yarn install")
DC-->>V: Environment with Node.js, TypeScript, Yarn ready
V-->>D: Container is ready for development
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CONTRIBUTING.md (1)
42-61: Addition of the Development Containers section is clear and helpful.
The new section gives detailed, step-by-step instructions to set up a consistent development environment using dev containers. Consider adding a link to the official dev container documentation for those who want to dive deeper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
dist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.mapdist/licenses.txtis excluded by!**/dist/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.devcontainer/devcontainer.json(1 hunks).github/workflows/activation.yml(1 hunks).github/workflows/build-tests-mac.yml(1 hunks).github/workflows/build-tests-windows.yml(3 hunks).github/workflows/cloud-runner-ci-pipeline.yml(2 hunks)CONTRIBUTING.md(1 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/activation.yml
🔇 Additional comments (10)
.devcontainer/devcontainer.json (1)
1-10: Review of new devcontainer configuration: Basic structure looks good.
The container is named and tagged with a proper Node.js/TypeScript image, and the specified feature for Node.js appears correctly provided.CONTRIBUTING.md (1)
38-41: General review of CONTRIBUTING.md formatting updates.
The existing sections have been maintained with only minor formatting improvements..github/workflows/build-tests-mac.yml (1)
58-63: UNITY_LICENSE environment variable added correctly.
The newly added environment variable on line 62 aligns with the PR objective and is consistently formatted with the other secrets. Ensure that the corresponding secret is properly configured in the repository’s settings.package.json (1)
31-32: Dependency version updates are noted.
Upgrading "@actions/cache" to "^4.0.0" and "@actions/core" to "^1.11.1" should help keep the workflows up to date with recent improvements. Please verify that these new versions are fully compatible with the rest of the CI setup and that all workflow steps have been tested..github/workflows/build-tests-windows.yml (3)
65-69: Consistency check for UNITY_LICENSE on Windows build step.
The addition of UNITY_LICENSE on line 68 ensures that the Windows build environment receives the same secret as the MacOS configuration. This is consistent and clear.
89-94: Verification of UNITY_LICENSE on the first retry build step.
The inclusion of UNITY_LICENSE on line 93 for the first retry is appropriate. Double-check that all secrets are set for every step that uses them to avoid intermittent build issues.
113-118: Verification of UNITY_LICENSE on the second retry build step.
Similarly, adding UNITY_LICENSE on line 117 ensures that all fallback build attempts have access to the Unity license, which maintains consistency across the workflow..github/workflows/cloud-runner-ci-pipeline.yml (3)
193-196: Enhancement: Added AWS and Token Environment Variables in Unity Build StepThe new environment variables (AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, GIT_PRIVATE_TOKEN, and GITHUB_TOKEN) have been added within the "unity-build" step of the buildTargetTests job. This ensures that the build has access to necessary credentials and tokens for AWS and Git operations. Please verify that these secrets are defined in your repository settings and that similar changes (if any) in other workflow files are consistent.
203-204: Review: CP Command Formatting ChangeThere is a minor change indicated by a "~" following the cp command that copies the build artifact from the cache. It appears to be a formatting or a minor whitespace update. Please confirm that this change is intentional and that the cp command correctly handles the source and destination paths in your CI environment.
🧰 Tools
🪛 actionlint (1.7.4)
203-203: property "cache_key" is not defined in object type {androidversioncode: string; buildversion: string; engineexitcode: string; volume: string}
(expression)
205-209: Upgrade: Artifact Upload Action to v4The actions/upload-artifact action has been updated from v3 to v4. This upgrade should provide performance improvements and the latest features. Ensure that this new version is compatible with your workflow configurations across all jobs—especially given the coordinated updates in other CI files—and test that no unintended breaking changes occur.
🧰 Tools
🪛 actionlint (1.7.4)
208-208: property "build_artifact" is not defined in object type {androidversioncode: string; buildversion: string; engineexitcode: string; volume: string}
(expression)
| "postCreateCommand": { | ||
| "Install dependencies": "yarn install" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Verify the format of postCreateCommand.
The current format uses an object with a label ("Install dependencies") mapping to the command. According to standard devcontainer.json schema, postCreateCommand is typically defined as a string or an array of strings. Consider revising it to a simpler format, for example:
- "postCreateCommand": {
- "Install dependencies": "yarn install"
- }
+ "postCreateCommand": "yarn install"This change helps ensure compatibility with standard dev container tooling.
|
If this is just for installing Node and Yarn, then we already use Volta. Have you considered using Volta as prescribed? Can you speak for the upsides of using devcontainer instead? What needs to be installed to use the config? |
I followed the instructions from CONTRIBUTING.md.
Des containers allows you to deploy a project and its dependencies in a docker container without installing anything on the host system.
Only docker and compatible ide (vscode for me) Is it possible to test locally that the code successfully builds unity projects? |
Yes, you should be able to run with act (though it's been a long time since I last did it) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #689 +/- ##
=======================================
Coverage 37.00% 37.00%
=======================================
Files 77 77
Lines 3129 3129
Branches 646 646
=======================================
Hits 1158 1158
Misses 1828 1828
Partials 143 143 |
|
I'm not super against this approach per se. However, I'm not sure if a much heavier stack (i.e. you now docker and an IDE extension, just to work with the repo) is helpful here. Especially since if you want the action to eventually run locally, you in addition need dind or docker service to run on the side - because it isn't always trivial to run a docker container from within another docker container. And I'm not sure how much dev containers support that kind of thing. When looking at this PR I'm also not sure about these points:
|
Egorrko
force-pushed
the
devcontainers
branch
from
ab4c1db to
fae7560
Compare
Changes
Successful Workflow Run Link
PRs don't have access to secrets so you will need to provide a link to a successful run of the workflows from your own
repo.
Checklist
code of conduct
in the documentation repo)
Summary by CodeRabbit