-
Notifications
You must be signed in to change notification settings - Fork 51
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
base: main
Are you sure you want to change the base?
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
UpdaterBuilder
to launch the container asdependabot
with an interactive shell - Added
{ recursive: true }
tofs.mkdirSync
in 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-certificates
logic inContainerService.run
isn’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/sh
without-c
or 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 theCmd
override to use the image’s default entrypoint.
Cmd: ['/bin/sh'],
['/bin/sh', '-c', cmd], | ||
'dependabot' | ||
) | ||
} |
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() |
Copilot uses AI. Check for mistakes.
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
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