-
Notifications
You must be signed in to change notification settings - Fork 2
Fix Playwright dev container feature to install browsers under non-root user #82
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
Fix Playwright dev container feature to install browsers under non-root user #82
Conversation
Co-authored-by: schlich <[email protected]>
Co-authored-by: schlich <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the Playwright dev container feature to install browsers under the appropriate non-root user instead of always installing under root, which was causing permission issues in typical dev container environments.
- Implements user detection logic following dev container patterns to identify the target user
- Updates installation process to run as the determined user using
suwith preserved environment - Adds comprehensive test coverage for the user installation functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/playwright/install.sh | Adds user detection logic and conditional installation process based on determined user |
| test/playwright/test_user_install.sh | New test script to validate basic Playwright functionality |
| test/playwright/scenarios.json | Adds test scenario for user installation validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
@copilot it's not clear what the "auto"/"automatic" users mean, and i think falling back to root is an appropriate measure if there's no user set. Remove the fallback options unless necessary. You could add some stronger tests including checking file permissions/ownership and trying commands as a different user. Also, investigate devcontainer lifecycle hooks and determine if simply moving all or some of the installation command to one of these hooks might be a more effective or efficient solution. |
- Remove complex user determination fallback logic - Use postCreateCommand to install browsers as the user automatically - Install only system dependencies in install.sh (as root) - Add stronger tests checking file ownership and permissions - This approach is simpler and more aligned with devcontainer lifecycle Co-authored-by: schlich <[email protected]>
I've simplified the implementation by using devcontainer lifecycle hooks instead of complex user detection logic (commit c72074d). Changes made:
Why this is better:
The approach separates concerns: system dependencies (root) vs. user-specific browser binaries (user context). Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot examine the failing test for the "single browser" test. I'm guessing it's invoking the test with the wrong user, adjust the test accordingly if appropriate |
- Remove postCreateCommand to ensure tests work correctly - Simplify to check only _REMOTE_USER (no complex fallbacks) - If _REMOTE_USER is set and exists, install as that user - Otherwise install as root (works for test scenarios) - This approach works for both tests and real devcontainer usage Co-authored-by: schlich <[email protected]>
Fixed in commit d195d7f. The issue was that Reverted to installing browsers in
This works for both test scenarios (no _REMOTE_USER, runs as root) and real devcontainers (_REMOTE_USER set, runs as user). |
Problem
The Playwright dev container feature was installing browsers under
/root/.cache/ms-playwrightinstead of the appropriate non-root user's home directory (e.g.,/home/vscode/.cache/ms-playwright). This caused permission issues and prevented the feature from working correctly in typical dev container setups where users operate as non-root.Solution
Simplified the installation script to check for
_REMOTE_USERand install browsers as that user when available, with a clean fallback to root for test scenarios.Key Changes
Simplified install.sh: Removed complex user determination logic and fallback mechanisms
_REMOTE_USERenvironment variableEnhanced Testing: Added comprehensive test coverage with file ownership and permission checks to validate proper user installation
How It Works
Before:
After:
_REMOTE_USERis set (typical devcontainer): browsers install in/home/vscode/.cache/ms-playwright_REMOTE_USERis not set (test scenarios): browsers install in/root/.cache/ms-playwrightBenefits
Fixes #81
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.