-
Notifications
You must be signed in to change notification settings - Fork 62
Run update-ca-certificates as root
#1505
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
Run update-ca-certificates as root
#1505
Conversation
1bdad52 to
54934a6
Compare
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 ensures update-ca-certificates runs as root before executing Dependabot commands under the dependabot user in the updater container, and fixes a test directory creation call.
- Split command execution into root vs. non-root phases in the container service
- Updated
UpdaterBuilderto launch the container asdependabotwith an interactive shell - Added
{ recursive: true }tofs.mkdirSyncin the integration test
Reviewed Changes
Copilot reviewed 3 out of 9 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/updater-builder.ts | Set container User to dependabot and override Cmd/Tty flags |
| src/container-service.ts | Introduce execCommand helper, run CA update as root, then user |
| tests/updater-builder-integration.test.ts | Add recursive flag to mkdirSync to avoid missing parent folders |
Comments suppressed due to low confidence (2)
tests/updater-builder-integration.test.ts:44
- The new root-phase
update-ca-certificateslogic inContainerService.runisn’t covered by existing integration tests. Consider adding a test that verifies the CA certificates step succeeds when running as root.
fs.mkdirSync(workingDirectory, {recursive: true})
src/updater-builder.ts:55
- Overriding the container command to just
/bin/shwithout-cor a no-op entrypoint leaves the shell open and may prevent the intended workflow. Consider specifying a simple no-op (e.g.,['/bin/sh', '-c', 'while true; do sleep 1; done']) or removing theCmdoverride to use the image’s default entrypoint.
Cmd: ['/bin/sh'],
| ['/bin/sh', '-c', cmd], | ||
| 'dependabot' | ||
| ) | ||
| } |
Copilot
AI
Jun 25, 2025
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.
In the dependabot branch, the container process (an interactive shell) is never stopped or waited on before removal, which can lead to resource leaks or hanging. Add an explicit container.stop() or a final container.wait() in the dependabot path once all execCommand calls complete.
| } | |
| } | |
| // Stop the container after all commands are executed | |
| await container.stop() |
54934a6 to
b518b30
Compare
|
Not sure why the difference in dist check is failing. I am running EDIT: I'm running Node 24 locally, and GitHub Actions is running Node 20. For some reason these were giving different results. |
b518b30 to
866cefc
Compare
866cefc to
ecfa025
Compare
jpinz
left a comment
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.
lgtm
This is equivalent to dependabot/cli#466
Required by dependabot/dependabot-core#9627