Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork928
allow for docker registry password to be a file#2419
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 ourterms of service andprivacy 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?
allow for docker registry password to be a file#2419
Uh oh!
There was an error while loading.Please reload this page.
Conversation
🦋 Changeset detectedLatest commit:b80c015 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
Not sure what this means?Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
coderabbitaibot commentedAug 19, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from theIntegrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
SupportNeed help? Create a ticket on oursupport page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File ( |
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.
Actionable comments posted: 2
🧹 Nitpick comments (3)
apps/supervisor/src/workloadManager/docker.ts (3)
11-11:Avoid synchronous disk I/O on the hot path (optional)Using readFileSync will block the event loop. Given this runs in the supervisor, impact is likely small, but consider either caching the password at startup or switching to async fs.promises.readFile with an async auth path.
280-307:Optional: cache password after first resolutionIf create() is called frequently, re-reading the same file each time is unnecessary. You can memoize the computed password (including the env string path case) to avoid repeated I/O.
51-61:Add basic tests for password resolution pathsTests to cover: plain env value, file:// happy path, file:// missing file (should log and skip auth), trimming behavior. I can scaffold these if helpful.
Also applies to: 280-307
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
.changeset/red-moose-design.md(1 hunks)apps/supervisor/src/workloadManager/docker.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/supervisor/src/workloadManager/docker.ts
🧬 Code Graph Analysis (1)
apps/supervisor/src/workloadManager/docker.ts (1)
apps/supervisor/src/env.ts (1)
env(105-105)
🔇 Additional comments (4)
.changeset/red-moose-design.md (1)
1-6:Changelog entry LGTMClear and scoped. Patch level is appropriate for this behavior change.
apps/supervisor/src/workloadManager/docker.ts (3)
41-49:Credential presence logging LGTMGood clarity on whether registry auth will be attempted.
169-175:Image pull refactor LGTM (uses auth() method)Swapping to this.auth() keeps behavior consistent while computing auth lazily. Once auth() is made safe against throws, this will be robust.
51-61:No lingeringthis.authproperty accesses detected
- Verified with
rg -n -C2 -g '!**/dist/**' -P '\bthis\.auth\b(?!\s*\()'and found no matches.All call sites correctly invoke
this.auth().
| privateauth():Docker.AuthConfig|undefined{ | ||
| if(env.DOCKER_REGISTRY_USERNAME&&env.DOCKER_REGISTRY_PASSWORD&&env.DOCKER_REGISTRY_URL){ | ||
| return{ | ||
| username:env.DOCKER_REGISTRY_USERNAME, | ||
| password:env.DOCKER_REGISTRY_PASSWORD, | ||
| password:getDockerPassword(), | ||
| serveraddress:env.DOCKER_REGISTRY_URL, | ||
| }; | ||
| }else{ | ||
| this.logger.warn("🐋 No Docker registry credentials provided, skipping auth"); | ||
| } | ||
| returnundefined; | ||
| } |
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.
Synchronous throw from getDockerPassword can crash create() before tryCatch; handle errors inside auth()
this.auth() calls getDockerPassword(), which may throw. Because arguments are evaluated before awaiting tryCatch(this.docker.createImage(...)), any synchronous throw here will bypass tryCatch and bubble out of create(), potentially crashing the supervisor. Wrap resolution and guard against empty/undefined passwords.
Apply this diff:
private auth(): Docker.AuthConfig | undefined {- if (env.DOCKER_REGISTRY_USERNAME && env.DOCKER_REGISTRY_PASSWORD && env.DOCKER_REGISTRY_URL) {- return {- username: env.DOCKER_REGISTRY_USERNAME,- password: getDockerPassword(),- serveraddress: env.DOCKER_REGISTRY_URL,- };- }-- return undefined;+ if (!(env.DOCKER_REGISTRY_USERNAME && env.DOCKER_REGISTRY_PASSWORD && env.DOCKER_REGISTRY_URL)) {+ return undefined;+ }++ try {+ const password = getDockerPassword();+ if (!password) {+ this.logger.warn("🐋 Docker registry password resolved to empty/undefined, skipping auth", {+ username: env.DOCKER_REGISTRY_USERNAME,+ url: env.DOCKER_REGISTRY_URL,+ });+ return undefined;+ }++ return {+ username: env.DOCKER_REGISTRY_USERNAME,+ password,+ serveraddress: env.DOCKER_REGISTRY_URL,+ };+ } catch (error) {+ this.logger.error("🐋 Failed to resolve Docker registry password, skipping auth", { error });+ return undefined;+ } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| privateauth():Docker.AuthConfig|undefined{ | |
| if(env.DOCKER_REGISTRY_USERNAME&&env.DOCKER_REGISTRY_PASSWORD&&env.DOCKER_REGISTRY_URL){ | |
| return{ | |
| username:env.DOCKER_REGISTRY_USERNAME, | |
| password:env.DOCKER_REGISTRY_PASSWORD, | |
| password:getDockerPassword(), | |
| serveraddress:env.DOCKER_REGISTRY_URL, | |
| }; | |
| }else{ | |
| this.logger.warn("🐋 No Docker registry credentials provided, skipping auth"); | |
| } | |
| returnundefined; | |
| } | |
| privateauth():Docker.AuthConfig|undefined{ | |
| if(!(env.DOCKER_REGISTRY_USERNAME&&env.DOCKER_REGISTRY_PASSWORD&&env.DOCKER_REGISTRY_URL)){ | |
| returnundefined; | |
| } | |
| try{ | |
| constpassword=getDockerPassword(); | |
| if(!password){ | |
| this.logger.warn("🐋 Docker registry password resolved to empty/undefined, skipping auth",{ | |
| username:env.DOCKER_REGISTRY_USERNAME, | |
| url:env.DOCKER_REGISTRY_URL, | |
| }); | |
| returnundefined; | |
| } | |
| return{ | |
| username:env.DOCKER_REGISTRY_USERNAME, | |
| password, | |
| serveraddress:env.DOCKER_REGISTRY_URL, | |
| }; | |
| }catch(error){ | |
| this.logger.error("🐋 Failed to resolve Docker registry password, skipping auth",{ error}); | |
| returnundefined; | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/supervisor/src/workloadManager/docker.ts around lines 51 to 61, auth()currently calls getDockerPassword() directly which can synchronously throw andescape the outer tryCatch in create(); wrap the getDockerPassword() call in aninternal try/catch so auth() never throws (catch errors and return undefined oromit the password field), and additionally guard against empty/undefinedpassword values (if password is falsy return undefined or build the auth objectwithout the password) so create() cannot be crashed by a synchronous exceptionfrom getDockerPassword().| functiongetDockerPassword():string|undefined{ | ||
| if(!env.DOCKER_REGISTRY_PASSWORD){ | ||
| returnundefined | ||
| } | ||
| if(!env.DOCKER_REGISTRY_PASSWORD.startsWith("file://")){ | ||
| returnenv.DOCKER_REGISTRY_PASSWORD; | ||
| } | ||
| constpasswordPath=env.DOCKER_REGISTRY_PASSWORD.replace("file://",""); | ||
| console.debug( | ||
| JSON.stringify({ | ||
| message:"🔑 Reading docker password from file", | ||
| passwordPath, | ||
| }) | ||
| ); | ||
| try{ | ||
| constpassword=readFileSync(passwordPath,"utf8").trim(); | ||
| returnpassword; | ||
| }catch(error){ | ||
| console.error(`Failed to read docker password from file:${passwordPath}`,error); | ||
| thrownewError( | ||
| `Unable to read docker password from file:${errorinstanceofError ?error.message :"Unknown error" | ||
| }` | ||
| ); | ||
| } | ||
| } |
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.
🛠️ Refactor suggestion
Don’t throw on password file read failure; use structured logging and return undefined
Throwing here can crash the call site (see auth() comment). Prefer logging and returning undefined so the caller can skip auth. Also, use the project’s structured logger instead of console.* and fix minor style nits.
Apply this diff:
function getDockerPassword(): string | undefined { if (!env.DOCKER_REGISTRY_PASSWORD) {- return undefined+ return undefined; } if (!env.DOCKER_REGISTRY_PASSWORD.startsWith("file://")) { return env.DOCKER_REGISTRY_PASSWORD; } const passwordPath = env.DOCKER_REGISTRY_PASSWORD.replace("file://", "");- console.debug(- JSON.stringify({- message: "🔑 Reading docker password from file",- passwordPath,- })- );+ moduleLogger.debug("🔑 Reading docker password from file", { passwordPath }); try { const password = readFileSync(passwordPath, "utf8").trim(); return password; } catch (error) {- console.error(`Failed to read docker password from file: ${passwordPath}`, error);- throw new Error(- `Unable to read docker password from file: ${error instanceof Error ? error.message : "Unknown error"- }`- );+ moduleLogger.error("Failed to read docker password from file", {+ passwordPath,+ error,+ });+ return undefined; } }Add this module-level logger once near the top of the file (outside the selected range):
// Module-level logger for helpers outside the classconstmoduleLogger=newSimpleStructuredLogger("docker-workload-manager");
🤖 Prompt for AI Agents
In apps/supervisor/src/workloadManager/docker.ts around lines 280 to 307, thehelper currently uses console.debug/console.error and throws on failing to reada password file; change it to use the project’s module-level structured loggerand return undefined on failure instead of throwing so callers can skip auth.Add the suggested moduleLogger declaration once near the top of the file(outside this range): const moduleLogger = newSimpleStructuredLogger("docker-workload-manager"); then replace console.debugwith moduleLogger.debug(...) including the same message and passwordPath, and inthe catch block call moduleLogger.warn or moduleLogger.error with a structuredobject { message: "Failed to read docker password from file", passwordPath,error: error instanceof Error ? error.message : String(error) } and returnundefined instead of throwing; keep the same behavior for non-file passwords andundefined env values.
No description provided.