refactor: consolidate repository variables#609
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 32 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughReplaces numerous hard-coded product, binary, repo, and env-string literals with centralized config constants (pkg/config), introduces config path/name constants, and updates many codepaths to use these new constants (binary name, env keys, file paths, URLs); also removes several exported daemonclient flag/debug constants. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
skevetter
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/pro/login.go (1)
259-272:⚠️ Potential issue | 🟠 MajorThe dev fallback provider still hardcodes the old product metadata.
This branch now keys off
config.RepoSlug, butfallbackProvideris still pinned todevpod-pro,https://devpod.sh/..., and/usr/local/bin/devpod. In a renamed or forked build, the development-provider path will install a manifest that points at the wrong binary and branding.🛠️ Suggested direction
- Raw: []byte(fallbackProvider), + Raw: []byte(buildFallbackProvider()),Build that manifest from
config.ProReleaseName,config.BinaryName, andconfig.WebsiteAssetsURLso the dev-only path stays aligned with the rest of this refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pro/login.go` around lines 259 - 272, The dev fallback provider manifest currently uses hardcoded product metadata (fallbackProvider) while the branch checks config.RepoSlug; update the manifest generation so AddProviderRaw receives a manifest built from config.ProReleaseName, config.BinaryName, and config.WebsiteAssetsURL instead of the old "devpod-pro"/"https://devpod.sh"/"/usr/local/bin/devpod" literals; locate where fallbackProvider is defined and replace its static string with a small builder that composes the provider manifest using those three config values, then pass the resulting raw bytes into workspace.AddProviderRaw (used in the branch that checks cmd.ProviderSource and calls workspace.AddProviderRaw with devPodConfig and log).pkg/ide/jetbrains/generic.go (1)
112-117:⚠️ Potential issue | 🟠 MajorNamespace the JetBrains volume name with
BinaryNameas well.
getDownloadFolder()is now isolated underContainerDataDir, but the backing Docker volume is stilldevpod-<id>. Rebranded builds will keep sharing the same volume, which defeats the refactor and can leak/corrupt cached IDE downloads across binaries.🐛 Proposed fix
func (o *GenericJetBrainsServer) GetVolume() string { - return fmt.Sprintf("type=volume,src=devpod-%s,dst=%s", o.options.ID, o.getDownloadFolder()) + return fmt.Sprintf("type=volume,src=%s-%s,dst=%s", config2.BinaryName, o.options.ID, o.getDownloadFolder()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ide/jetbrains/generic.go` around lines 112 - 117, GetVolume currently constructs the Docker volume name as "devpod-<id>" which can collide across different binary builds; update GenericJetBrainsServer.GetVolume to include the binary namespace (o.options.BinaryName) in the volume name so it matches the per-binary isolation intent of getDownloadFolder (and the ContainerDataDir pattern). In short: modify GenericJetBrainsServer.GetVolume to incorporate o.options.BinaryName along with o.options.ID (e.g., "devpod-<BinaryName>-<ID>") so the Docker volume is namespaced per binary and won't share cached IDE downloads across rebranded builds.
🧹 Nitpick comments (9)
pkg/gitcredentials/gitcredentials.go (1)
18-19: Prefer keeping shared env-var keys inpkg/config.This constant is now consumed outside the git-credentials implementation, so declaring it here pulls unrelated callers into a lower-level package just to share a string. Moving it alongside the other env-var constants would keep dependency direction cleaner.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gitcredentials/gitcredentials.go` around lines 18 - 19, The constant EnvGitHelperPort is declared in pkg/gitcredentials (symbol: EnvGitHelperPort) but is consumed by callers outside that package; move this env-var key into the shared pkg/config where other env constants live, remove the EnvGitHelperPort declaration from gitcredentials.go, and update all call sites to import and reference the new symbol (e.g., config.EnvGitHelperPort) so dependency direction remains correct and no unrelated packages must depend on pkg/gitcredentials.cmd/pro/start.go (1)
97-116: Derive the printed CLI commands from config too.This makes the install defaults configurable, but the guidance later in this file still hardcodes
devpod pro .... Ifconfig.BinaryNameorconfig.ProReleaseNameever changes, the installer will succeed and then tell users to run commands that no longer exist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pro/start.go` around lines 97 - 116, The post-install printed CLI guidance still hardcodes "devpod pro ..."; update all printed/example commands in this file to derive from config.BinaryName and config.ProReleaseName instead of literal strings. Locate where startCmd (and any success/next-step log/print statements) prints example commands and replace hardcoded "devpod" or "devpod pro" tokens with fmt.Sprintf("%s %s", config.BinaryName, config.ProReleaseName) or similar formatted strings so examples always reflect config.BinaryName and config.ProReleaseName values.pkg/dockercredentials/dockercredentials.go (1)
128-128: Usepkgconfig.BinaryNameconsistently for the helper name.Lines 95–128 should both reference
pkgconfig.BinaryNameto avoid duplication and ensure the credentials store name and helper binary name remain consistent. Currently, line 95 hardcodes"docker-credential-devpod"while line 128 usespkgconfig.BinaryName. Although both resolve to"devpod"today, aligning them follows DRY principles and makes the code more maintainable.Suggested fix
- helperName := "docker-credential-devpod" + helperName := "docker-credential-" + pkgconfig.BinaryName if runtime.GOOS == windowsOS { helperName += ".cmd" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dockercredentials/dockercredentials.go` at line 128, Replace the hardcoded helper name "docker-credential-devpod" with the constant pkgconfig.BinaryName so the credentials store name and helper binary name are consistent; update the earlier assignment that sets the credentials helper (currently using the literal "docker-credential-devpod") to use pkgconfig.BinaryName instead, ensuring dockerConfig.CredentialsStore and any other reference both use pkgconfig.BinaryName.pkg/ide/ideparse/parse.go (1)
40-214: Factor asset URL construction into one helper.This table now duplicates
config.WebsiteAssetsURL + "/..."dozens of times. A small helper would remove the repetition and keep trailing-slash handling in one place.♻️ Example cleanup
+func asset(name string) string { + return strings.TrimRight(config.WebsiteAssetsURL, "/") + "/" + name +} + var AllowedIDEs = []AllowedIDE{ { Name: config.IDEVSCode, DisplayName: "VS Code", Options: vscode.Options, - Icon: config.WebsiteAssetsURL + "/vscode.svg", + Icon: asset("vscode.svg"), Group: config.IDEGroupPrimary, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ide/ideparse/parse.go` around lines 40 - 214, The AllowedIDEs slice duplicates config.WebsiteAssetsURL + "/..." for Icon/IconDark entries; add a small helper function (e.g., assetURL or MakeAssetURL) that normalizes/trims trailing slashes on config.WebsiteAssetsURL and joins a path, then replace all occurrences in AllowedIDEs where Icon and IconDark are set to call that helper (update entries like the ones referencing config.WebsiteAssetsURL + "/vscode.svg", "/none_dark.svg", etc.). Ensure the helper is used for both Icon and IconDark where present and keep semantics unchanged for experimental/group/options fields.pkg/ide/rstudio/rstudio.go (1)
52-53: Keep the RStudio state path config-driven too.
downloadFoldernow followsContainerDataDir, butdataFolderis still pinned to/usr/local/share/devpod/.... Rebranded builds will keep writing RStudio state under the old product name, so this refactor is only partially applied here.♻️ Proposed fix
const ( DefaultServerPort = 8787 downloadFolder = agent.ContainerDataDir + "/rstudio-server" - dataFolder = "/usr/local/share/devpod/rstudio-server/data" + dataFolder = "/usr/local/share/" + config.BinaryName + "/rstudio-server/data" // rstudioConfigFolder is where RStudio expects configuration. rstudioConfigFolder = "/etc/rstudio" preferencesFile = "rstudio-prefs.json"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/ide/rstudio/rstudio.go` around lines 52 - 53, dataFolder is still hard-coded to "/usr/local/share/devpod/rstudio-server/data" while downloadFolder uses agent.ContainerDataDir; update dataFolder to be config-driven as well (e.g., build it from agent.ContainerDataDir plus the "rstudio-server/data" suffix) so both paths follow the same container data dir; change the declaration of dataFolder in rstudio.go to use agent.ContainerDataDir (matching downloadFolder's pattern) and update any code that references dataFolder if it assumes the old absolute path.cmd/agent/container/ssh_server.go (1)
18-18: Consider keepingBaseLogDirunexported or documenting mutability.Changing
BaseLogDirfrom aconstto avaris necessary to initialize it fromagent.ContainerDataDir. However, as a public variable, it can now be modified at runtime, which could lead to unintended side effects.Consider either:
- Making it unexported (
baseLogDir) if external access isn't required, or- Adding a comment documenting that this value should not be modified after initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/agent/container/ssh_server.go` at line 18, The exported variable BaseLogDir was changed from a const to a var and is now mutable at runtime; either make it unexported (rename to baseLogDir) if no external package needs it, or keep it exported but add a clear package-level comment above BaseLogDir stating it is initialized from agent.ContainerDataDir and must not be modified after initialization; update any references to BaseLogDir to use the new name if you choose to unexport it, and ensure initialization still happens where agent.ContainerDataDir is available.pkg/telemetry/collect.go (1)
149-154: Consider updating the hardcoded runner path check.Line 151 still uses the hardcoded path
/var/lib/loft/devpodto detect if running on a pro runner. This path contains "devpod" which may be inconsistent with the refactoring goal if the binary/product name changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/telemetry/collect.go` around lines 149 - 154, Replace the hardcoded "/var/lib/loft/devpod" check in the os.Getwd() block with a configurable or more stable detection: read a constant or env var (e.g., PRO_RUNNER_PATH or a package-level proRunnerPath) or match on a broader, product-stable fragment (e.g., "/var/lib/loft") instead of "devpod"; update the wd, wdErr and isPro logic in pkg/telemetry/collect.go so the check uses that configured value/fragment to determine isPro rather than the literal "/var/lib/loft/devpod".pkg/agent/workspace.go (2)
156-173: Test script content still uses hardcoded "DevPod" string.The test file name correctly uses
config.BinaryName + "_test.sh", but the script content (line 159) outputs "DevPod" and the validation (line 172-173) checks for the literal string "DevPod". If the binary is renamed, the test will still pass because it checks for "DevPod" rather than a dynamic value.This may be acceptable if "DevPod" is the product name that shouldn't change, but it's worth confirming this is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/agent/workspace.go` around lines 156 - 173, The test script writes and executes testFile but hardcodes the string "DevPod" and compares output to that literal; update the written script to echo config.BinaryName (or another dynamic expected value) and change the validation to compare to strings.TrimSpace(string(out)) == config.BinaryName (or the chosen expected variable) so the check follows config.BinaryName; modify the code around testFile creation and the comparison that uses exec.Command(...).Output() and strings.TrimSpace to use the dynamic expected value instead of "DevPod".
31-36: Add a clarifying comment explaining the backward compatibility intent for the hardcoded/home/devpod/search location.Line 32 includes
/home/devpod/+config.ConfigDirName+/agentas a search location. This hardcoded path appears intentional as part of the backward compatibility search locations (alongside/opt/devpod/agentand/var/lib/devpod/agent). However, the mixing of a hardcoded username with the dynamicConfigDirNameshould be documented with a comment explaining this is for backward compatibility with existing installations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/agent/workspace.go` around lines 31 - 36, Add a short clarifying comment above the extraSearchLocations slice explaining that the entry "/home/devpod/"+config.ConfigDirName+"/agent" is intentionally hardcoded to preserve backward compatibility with legacy installs that used the devpod home path; mention that other entries ("/opt/devpod/agent", "/var/lib/devpod/agent", and ContainerDataDir+"/agent") are additional standard locations and that the hardcoded username is deliberate rather than an oversight. Ensure the comment references the extraSearchLocations variable and the use of config.ConfigDirName and ContainerDataDir so future maintainers understand the compatibility intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/agent/container/daemon.go`:
- Around line 28-29: DaemonConfigPath is hardcoded to
"/var/run/secrets/devpod/daemon_config" and should follow the centralized binary
name like RootDir does; change DaemonConfigPath to be derived from the shared
binary name constant (e.g. use agent.BinaryName) instead of "devpod". Build the
path with path.Join("/var/run/secrets", agent.BinaryName, "daemon_config") (or
fmt.Sprintf) and update the DaemonConfigPath assignment accordingly so the
secret mount location moves with the binary name.
In `@cmd/up.go`:
- Around line 1565-1566: The command string currently interpolates signingKey
directly into fmt.Sprintf(config.BinaryName+" agent git-ssh-signature-helper
%s", signingKey), which breaks when signingKey contains spaces; change the
interpolation to quote the signingKey using shellescape.QuoteCommand(signingKey)
(i.e., pass shellescape.QuoteCommand(signingKey) as the %s argument) and add the
shellescape import if missing so the remote side receives the signing key as a
single, properly quoted argument.
In `@pkg/daemon/agent/daemon.go`:
- Line 104: The installer now uses pkgconfig.BinaryName when calling daemon.New
(e.g., service, err := daemon.New(pkgconfig.BinaryName, ...)) which can diverge
from the historical "devpod" unit; modify the install/remove flow to detect and
preserve/migrate the legacy "devpod" service before switching identifiers:
update the code around daemon.New and RemoveDaemon to first check for an
existing service named "devpod" (probe via daemon.Exists or equivalent), and if
found either reuse that unit name or perform a migration/rename step so the old
unit is not left behind; ensure both the creation path (daemon.New) and the
removal path (RemoveDaemon) handle the legacy "devpod" identifier in addition to
pkgconfig.BinaryName.
In `@pkg/devcontainer/config/result.go`:
- Line 10: The UserLabel constant (const UserLabel = pkgconfig.BinaryName +
".user") and uses of customizations[pkgconfig.BinaryName] make persisted keys
dependent on BinaryName; instead preserve backward compatibility by supporting
the legacy "devpod" key alongside the branded key or by defining a canonical
constant for the key (e.g., fallback to "devpod" when
customizations[pkgconfig.BinaryName] is absent). Update places that read/write
customizations and labels (references to UserLabel and
customizations[BinaryName]) to first attempt the branded key and then fall back
to "devpod" (or unify both to a single canonical constant) so existing
customizations.devpod and devpod.user continue to be recognized. Ensure both
read and write paths use this fallback logic so stored data remains stable
across branding changes.
In `@pkg/loftconfig/config.go`:
- Around line 13-20: The failure messages still hard-code "devpod pro login"
even though the command is built from pkgconfig.BinaryName; update any
error/usage strings that refer to "devpod pro login" to use pkgconfig.BinaryName
dynamically (e.g., fmt.Sprintf("%s pro login", pkgconfig.BinaryName)) so the
logged/help text matches the actual binary executed (locate the exec.Command
construction using pkgconfig.BinaryName and replace the hardcoded references
near where cmd is run and errors are reported).
In `@pkg/upgrade/upgrade.go`:
- Line 74: The upgrade messages in Upgrade still hardcode "devpod" even though
detectRelease now uses config.RepoSlug; update the up-to-date and success
log/print statements in the Upgrade function to reference the parsed repo (the
variable repo returned by selfupdate.ParseSlug(config.RepoSlug) or directly
config.RepoSlug) instead of the literal "devpod" so messages reflect the actual
repository used by detectRelease.
In `@pkg/workspace/machine.go`:
- Line 262: The legacy shared-machine lookup currently builds legacyMachineName
from config.BinaryName, which breaks detection when the binary was renamed;
change the fallback to use the historical literal prefix (e.g.
"devpod-shared-"+provider) for legacyMachineName while keeping the newly
generated name (config.BinaryName+"-shared-"+provider) for current use so
existing devpod-shared-<provider> machines are still detected and reused.
---
Outside diff comments:
In `@cmd/pro/login.go`:
- Around line 259-272: The dev fallback provider manifest currently uses
hardcoded product metadata (fallbackProvider) while the branch checks
config.RepoSlug; update the manifest generation so AddProviderRaw receives a
manifest built from config.ProReleaseName, config.BinaryName, and
config.WebsiteAssetsURL instead of the old
"devpod-pro"/"https://devpod.sh"/"/usr/local/bin/devpod" literals; locate where
fallbackProvider is defined and replace its static string with a small builder
that composes the provider manifest using those three config values, then pass
the resulting raw bytes into workspace.AddProviderRaw (used in the branch that
checks cmd.ProviderSource and calls workspace.AddProviderRaw with devPodConfig
and log).
In `@pkg/ide/jetbrains/generic.go`:
- Around line 112-117: GetVolume currently constructs the Docker volume name as
"devpod-<id>" which can collide across different binary builds; update
GenericJetBrainsServer.GetVolume to include the binary namespace
(o.options.BinaryName) in the volume name so it matches the per-binary isolation
intent of getDownloadFolder (and the ContainerDataDir pattern). In short: modify
GenericJetBrainsServer.GetVolume to incorporate o.options.BinaryName along with
o.options.ID (e.g., "devpod-<BinaryName>-<ID>") so the Docker volume is
namespaced per binary and won't share cached IDE downloads across rebranded
builds.
---
Nitpick comments:
In `@cmd/agent/container/ssh_server.go`:
- Line 18: The exported variable BaseLogDir was changed from a const to a var
and is now mutable at runtime; either make it unexported (rename to baseLogDir)
if no external package needs it, or keep it exported but add a clear
package-level comment above BaseLogDir stating it is initialized from
agent.ContainerDataDir and must not be modified after initialization; update any
references to BaseLogDir to use the new name if you choose to unexport it, and
ensure initialization still happens where agent.ContainerDataDir is available.
In `@cmd/pro/start.go`:
- Around line 97-116: The post-install printed CLI guidance still hardcodes
"devpod pro ..."; update all printed/example commands in this file to derive
from config.BinaryName and config.ProReleaseName instead of literal strings.
Locate where startCmd (and any success/next-step log/print statements) prints
example commands and replace hardcoded "devpod" or "devpod pro" tokens with
fmt.Sprintf("%s %s", config.BinaryName, config.ProReleaseName) or similar
formatted strings so examples always reflect config.BinaryName and
config.ProReleaseName values.
In `@pkg/agent/workspace.go`:
- Around line 156-173: The test script writes and executes testFile but
hardcodes the string "DevPod" and compares output to that literal; update the
written script to echo config.BinaryName (or another dynamic expected value) and
change the validation to compare to strings.TrimSpace(string(out)) ==
config.BinaryName (or the chosen expected variable) so the check follows
config.BinaryName; modify the code around testFile creation and the comparison
that uses exec.Command(...).Output() and strings.TrimSpace to use the dynamic
expected value instead of "DevPod".
- Around line 31-36: Add a short clarifying comment above the
extraSearchLocations slice explaining that the entry
"/home/devpod/"+config.ConfigDirName+"/agent" is intentionally hardcoded to
preserve backward compatibility with legacy installs that used the devpod home
path; mention that other entries ("/opt/devpod/agent", "/var/lib/devpod/agent",
and ContainerDataDir+"/agent") are additional standard locations and that the
hardcoded username is deliberate rather than an oversight. Ensure the comment
references the extraSearchLocations variable and the use of config.ConfigDirName
and ContainerDataDir so future maintainers understand the compatibility intent.
In `@pkg/dockercredentials/dockercredentials.go`:
- Line 128: Replace the hardcoded helper name "docker-credential-devpod" with
the constant pkgconfig.BinaryName so the credentials store name and helper
binary name are consistent; update the earlier assignment that sets the
credentials helper (currently using the literal "docker-credential-devpod") to
use pkgconfig.BinaryName instead, ensuring dockerConfig.CredentialsStore and any
other reference both use pkgconfig.BinaryName.
In `@pkg/gitcredentials/gitcredentials.go`:
- Around line 18-19: The constant EnvGitHelperPort is declared in
pkg/gitcredentials (symbol: EnvGitHelperPort) but is consumed by callers outside
that package; move this env-var key into the shared pkg/config where other env
constants live, remove the EnvGitHelperPort declaration from gitcredentials.go,
and update all call sites to import and reference the new symbol (e.g.,
config.EnvGitHelperPort) so dependency direction remains correct and no
unrelated packages must depend on pkg/gitcredentials.
In `@pkg/ide/ideparse/parse.go`:
- Around line 40-214: The AllowedIDEs slice duplicates config.WebsiteAssetsURL +
"/..." for Icon/IconDark entries; add a small helper function (e.g., assetURL or
MakeAssetURL) that normalizes/trims trailing slashes on config.WebsiteAssetsURL
and joins a path, then replace all occurrences in AllowedIDEs where Icon and
IconDark are set to call that helper (update entries like the ones referencing
config.WebsiteAssetsURL + "/vscode.svg", "/none_dark.svg", etc.). Ensure the
helper is used for both Icon and IconDark where present and keep semantics
unchanged for experimental/group/options fields.
In `@pkg/ide/rstudio/rstudio.go`:
- Around line 52-53: dataFolder is still hard-coded to
"/usr/local/share/devpod/rstudio-server/data" while downloadFolder uses
agent.ContainerDataDir; update dataFolder to be config-driven as well (e.g.,
build it from agent.ContainerDataDir plus the "rstudio-server/data" suffix) so
both paths follow the same container data dir; change the declaration of
dataFolder in rstudio.go to use agent.ContainerDataDir (matching
downloadFolder's pattern) and update any code that references dataFolder if it
assumes the old absolute path.
In `@pkg/telemetry/collect.go`:
- Around line 149-154: Replace the hardcoded "/var/lib/loft/devpod" check in the
os.Getwd() block with a configurable or more stable detection: read a constant
or env var (e.g., PRO_RUNNER_PATH or a package-level proRunnerPath) or match on
a broader, product-stable fragment (e.g., "/var/lib/loft") instead of "devpod";
update the wd, wdErr and isPro logic in pkg/telemetry/collect.go so the check
uses that configured value/fragment to determine isPro rather than the literal
"/var/lib/loft/devpod".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 080948b7-cf13-412a-98d7-1b28fd758f03
📒 Files selected for processing (60)
cmd/agent/container/daemon.gocmd/agent/container/setup.gocmd/agent/container/ssh_server.gocmd/agent/workspace/up.gocmd/build.gocmd/flags/flags.gocmd/helper/get_workspace_config.gocmd/pro/daemon/start.gocmd/pro/login.gocmd/pro/pro.gocmd/pro/provider/provider.gocmd/pro/start.gocmd/provider/list-default-providers.gocmd/provider/set_options.gocmd/provider/update.gocmd/root.gocmd/up.gohack/pro/main.gopkg/agent/agent.gopkg/agent/binary.gopkg/agent/workspace.gopkg/client/clientimplementation/daemonclient/client.gopkg/client/clientimplementation/daemonclient/up.gopkg/client/clientimplementation/proxy_client.gopkg/config/dir.gopkg/config/repo.gopkg/daemon/agent/daemon.gopkg/daemon/platform/daemon.gopkg/daemon/platform/socket.gopkg/devcontainer/config.gopkg/devcontainer/config/build.gopkg/devcontainer/config/prebuild.gopkg/devcontainer/config/result.gopkg/devcontainer/crane/run.gopkg/devcontainer/feature/features.gopkg/devcontainer/setup/setup.gopkg/dockercredentials/dockercredentials.gopkg/driver/docker/docker.gopkg/driver/kubernetes/driver.gopkg/driver/kubernetes/run.gopkg/gitcredentials/gitcredentials.gopkg/ide/fleet/fleet.gopkg/ide/ideparse/parse.gopkg/ide/jetbrains/generic.gopkg/ide/rstudio/rstudio.gopkg/ide/vscode/open.gopkg/ide/zed/zed.gopkg/ide/zed/zed_linux.gopkg/inject/download_urls.gopkg/loftconfig/config.gopkg/platform/client/client.gopkg/provider/dir.gopkg/provider/download.gopkg/ssh/config.gopkg/ssh/keys.gopkg/telemetry/collect.gopkg/ts/util.gopkg/upgrade/upgrade.gopkg/workspace/machine.gopkg/workspace/provider.go
💤 Files with no reviewable changes (1)
- pkg/client/clientimplementation/daemonclient/client.go
|
|
||
| // check if admin | ||
| service, err := daemon.New("devpod", "DevPod Agent Service", daemon.SystemDaemon) | ||
| service, err := daemon.New(pkgconfig.BinaryName, "DevPod Agent Service", daemon.SystemDaemon) |
There was a problem hiding this comment.
Preserve the legacy service identifier during upgrades.
Line 104 and Line 152 now derive the system service name from pkgconfig.BinaryName. If that value differs from the historical devpod, installs will create a second unit and RemoveDaemon will stop touching the one older versions created. Please keep probing or migrating the legacy devpod service before switching identifiers.
Also applies to: 152-152
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/daemon/agent/daemon.go` at line 104, The installer now uses
pkgconfig.BinaryName when calling daemon.New (e.g., service, err :=
daemon.New(pkgconfig.BinaryName, ...)) which can diverge from the historical
"devpod" unit; modify the install/remove flow to detect and preserve/migrate the
legacy "devpod" service before switching identifiers: update the code around
daemon.New and RemoveDaemon to first check for an existing service named
"devpod" (probe via daemon.Exists or equivalent), and if found either reuse that
unit name or perform a migration/rename step so the old unit is not left behind;
ensure both the creation path (daemon.New) and the removal path (RemoveDaemon)
handle the legacy "devpod" identifier in addition to pkgconfig.BinaryName.
| ) | ||
|
|
||
| const UserLabel = "devpod.user" | ||
| const UserLabel = pkgconfig.BinaryName + ".user" |
There was a problem hiding this comment.
Keep the devcontainer keys stable.
customizations[BinaryName] and BinaryName + ".user" turn persisted repo/container keys into branding-dependent values. Existing customizations.devpod blocks and devpod.user labels stop being read as soon as BinaryName changes, so custom settings and remote-user detection silently regress. Support the legacy "devpod" key at minimum, or keep it canonical.
🔁 Compatibility-safe approach
-const UserLabel = pkgconfig.BinaryName + ".user"
+const UserLabel = "devpod.user"
func GetDevPodCustomizations(parsedConfig *DevContainerConfig) *DevPodCustomizations {
- if parsedConfig.Customizations == nil ||
- parsedConfig.Customizations[pkgconfig.BinaryName] == nil {
+ if parsedConfig.Customizations == nil {
return &DevPodCustomizations{}
}
+ raw := parsedConfig.Customizations["devpod"]
+ if raw == nil {
+ raw = parsedConfig.Customizations[pkgconfig.BinaryName]
+ }
+ if raw == nil {
+ return &DevPodCustomizations{}
+ }
devPod := &DevPodCustomizations{}
- err := Convert(parsedConfig.Customizations[pkgconfig.BinaryName], devPod)
+ err := Convert(raw, devPod)Also applies to: 71-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/devcontainer/config/result.go` at line 10, The UserLabel constant (const
UserLabel = pkgconfig.BinaryName + ".user") and uses of
customizations[pkgconfig.BinaryName] make persisted keys dependent on
BinaryName; instead preserve backward compatibility by supporting the legacy
"devpod" key alongside the branded key or by defining a canonical constant for
the key (e.g., fallback to "devpod" when customizations[pkgconfig.BinaryName] is
absent). Update places that read/write customizations and labels (references to
UserLabel and customizations[BinaryName]) to first attempt the branded key and
then fall back to "devpod" (or unify both to a single canonical constant) so
existing customizations.devpod and devpod.user continue to be recognized. Ensure
both read and write paths use this fallback logic so stored data remains stable
across branding changes.
| } | ||
|
|
||
| repo := selfupdate.ParseSlug(defaultRepository) | ||
| repo := selfupdate.ParseSlug(config.RepoSlug) |
There was a problem hiding this comment.
Finish the rename in the upgrade messages.
detectRelease now follows config.RepoSlug, but the up-to-date and success paths in Upgrade still print "devpod". A renamed build will fetch the correct repo and then tell users it updated the wrong CLI.
💡 Suggested follow-up
- "devpod version %s is already up-to-date\n",
- release.Version(),
+ "%s version %s is already up-to-date\n",
+ config.BinaryName,
+ release.Version(),
...
- logger.Donef("updated devpod to version %s", release.Version())
+ logger.Donef("updated %s to version %s", config.BinaryName, release.Version())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/upgrade/upgrade.go` at line 74, The upgrade messages in Upgrade still
hardcode "devpod" even though detectRelease now uses config.RepoSlug; update the
up-to-date and success log/print statements in the Upgrade function to reference
the parsed repo (the variable repo returned by
selfupdate.ParseSlug(config.RepoSlug) or directly config.RepoSlug) instead of
the literal "devpod" so messages reflect the actual repository used by
detectRelease.
|
|
||
| func SingleMachineName(devPodConfig *config.Config, provider string, log log.Logger) string { | ||
| legacyMachineName := "devpod-shared-" + provider | ||
| legacyMachineName := config.BinaryName + "-shared-" + provider |
There was a problem hiding this comment.
The legacy shared-machine lookup no longer matches legacy machines.
Line 262 rebuilds legacyMachineName from config.BinaryName, so a renamed binary will stop detecting existing devpod-shared-<provider> machines and create a second shared machine instead of reusing the old one. The fallback should stay pinned to the historical prefix, while the newly generated name can remain config-driven.
Suggested fix
- legacyMachineName := config.BinaryName + "-shared-" + provider
+ legacyMachineName := "devpod-shared-" + provider📝 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.
Suggested change
| legacyMachineName := config.BinaryName + "-shared-" + provider | |
| legacyMachineName := "devpod-shared-" + provider |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/workspace/machine.go` at line 262, The legacy shared-machine lookup
currently builds legacyMachineName from config.BinaryName, which breaks
detection when the binary was renamed; change the fallback to use the historical
literal prefix (e.g. "devpod-shared-"+provider) for legacyMachineName while
keeping the newly generated name (config.BinaryName+"-shared-"+provider) for
current use so existing devpod-shared-<provider> machines are still detected and
reused.
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
Signed-off-by: Samuel K <skevetter@pm.me>
- Fix DaemonConfigPath to use config.BinaryName instead of hardcoded "devpod" - Quote signingKey with shellescape in setupGitSSHSignature to handle keys with spaces - Fix ProviderPrefix: remove RepoOwner prefix (CutPrefix matches against repo names, not full slugs) - Centralize DEVPOD_DISABLE_TELEMETRY and DEVPOD_WORKSPACE_CREDENTIALS_PORT env vars as constants
Create pkg/config/env.go with all environment variable constants using consistent EnvXxx naming convention. Remove scattered definitions from pkg/agent, pkg/credentials, pkg/gitcredentials, pkg/devcontainer, pkg/provider, and pkg/client/clientimplementation. Update all consumers.
Add pkg/config/product.go (ProductName, ProductNamePro, DaemonServiceDescription) and pkg/config/paths.go (IgnoreFileName, SSHSignatureHelperPath/Name, DockerCredentialHelperName, DevContainerResultPath) to eliminate hardcoded "DevPod Pro", "DevPod Agent Service", ".devpodignore", and related strings throughout the codebase.
Replace proxy_client DevPodXxx var aliases, devcontainer WorkspaceIDExtraEnvVar/WorkspaceUIDExtraEnvVar, config/build WorkspaceDaemonConfigExtraEnvVar, setup ResultLocation, gitsshsigning HelperScriptPath, and provider DEVPOD_OS/DEVPOD_ARCH/DEVPOD_LOG_LEVEL with their config.* originals throughout all callers.
Add EnvPrefix, EnvIDEPrefix, EnvProviderPrefix to config/env.go. Replace all hardcoded "DEVPOD_" string concatenation in cmd/ with config constants. Zero hardcoded DEVPOD_ strings remain outside config/env.go.
Introduce desktop/src/client/repo.ts as the single source of truth for repo/product names, URLs, and identifiers. Update all desktop source files to derive hardcoded strings (URLs, storage keys, container/K8s names, Flatpak IDs, provider prefixes) from repo.ts so renaming the fork only requires changing REPO_OWNER and REPO_NAME.
task rename -- <new-owner> <new-name> performs a full rename across: - Go source constants (pkg/config/repo.go) - TypeScript constants (desktop/src/client/repo.ts) - Go module path (go.mod + all imports) - Goreleaser, Tauri, Flatpak configs - GitHub Actions workflows - Taskfile.yml task rename:verify scans for any remaining old-name references.
Rename WORKSPACE_ID, MACHINE_ID, PROVIDER_ID, LOFT_PROJECT, DEVCONTAINER_ID etc. to WorkspaceID, MachineID, ProviderID, LoftProject, DevcontainerID. String values unchanged. WorkspaceSource renamed to WorkspaceSourceEnv to avoid collision with the existing WorkspaceSource type.
Move WORKSPACE_*, MACHINE_*, PROVIDER_*, LOFT_*, and DEVCONTAINER_ID constants from pkg/provider/env.go to the centralized pkg/config/env.go with EnvProviderXxx naming convention. Update all consumers.
skevetter
force-pushed
the
centralize-repo-config
branch
from
4803374 to
f891684
Compare
skevetter
force-pushed
the
centralize-repo-config
branch
2 times, most recently
from
abad50c to
987f0c0
Compare
…oolFalse Replace 52 occurrences of == "true", != "true", and == "false" string comparisons across cmd/ and pkg/ with config.BoolTrue and config.BoolFalse constants. Remove local envTrueValue constant and nolint:goconst directives.
skevetter
force-pushed
the
centralize-repo-config
branch
from
987f0c0 to
183f116
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
pkg/loftconfig/config.go (1)
24-28:⚠️ Potential issue | 🟠 MajorDo not include access keys in returned errors.
Line 25 includes
config.AccessKeyin the error payload, which risks secret leakage in logs and telemetry.Suggested patch
return fmt.Errorf( - "error executing '%s' command: %w, access key: %v, host: %v", + "error executing '%s' command: %w, host: %v", loginCmd, err, - config.AccessKey, config.Host, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/loftconfig/config.go` around lines 24 - 28, The error message built with fmt.Errorf currently includes config.AccessKey which leaks secrets; remove the AccessKey from the error payload and stop returning it (for example, drop config.AccessKey from the fmt.Errorf call in the code that constructs "error executing 'devpod pro login' command"), leaving only the error and non-secret fields (e.g., err and config.Host) or replace the key with a redacted placeholder if you must indicate presence, and ensure any related error formatting sites (the fmt.Errorf call) no longer reference config.AccessKey.desktop/src/views/Pro/Workspace/WorkspaceDetails.tsx (1)
440-443:⚠️ Potential issue | 🟡 MinorFinish container-name centralization in PodStatus.
Line 442 still hard-codes
"devpod", so pod reason/message resolution can break when the runtime container name differs from that literal.Suggested fix
- container.name === "devpod" && + container.name === CONTAINER_NAME && (container.state?.waiting?.reason || container.state?.terminated?.reason)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/views/Pro/Workspace/WorkspaceDetails.tsx` around lines 440 - 443, The code still uses the hard-coded string "devpod" when locating a container status; update the lookup to use the centralized container name value instead (e.g., reference the PodStatus/containerName constant or accessor you added) so containerStatus = podStatus.containerStatuses?.find(container => container.name === <centralizedContainerName> && (container.state?.waiting?.reason || container.state?.terminated?.reason)); ensure you import or reference the same symbol used elsewhere in PodStatus (the centralized container name or getter) and replace the literal in WorkspaceDetails.tsx so resolution works for different runtime container names.pkg/gitsshsigning/helper.go (1)
213-213:⚠️ Potential issue | 🟡 MinorInconsistent use of hardcoded helper name in removal logic.
Line 213 still uses the hardcoded string
"program = devpod-ssh-signature"while other locations (lines 25, 113) usepkgconfig.SSHSignatureHelperName. If the helper name changes, this removal logic will fail to match and clean up old entries.🔧 Proposed fix
- if !strings.HasPrefix(trimmed, "program = devpod-ssh-signature") { + if !strings.HasPrefix(trimmed, "program = "+pkgconfig.SSHSignatureHelperName) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/gitsshsigning/helper.go` at line 213, The removal logic is using a hardcoded string "program = devpod-ssh-signature" instead of the constant; update the check to use pkgconfig.SSHSignatureHelperName so the match follows the canonical helper name used elsewhere (replace the literal in the conditional that currently reads "program = devpod-ssh-signature" with a formatted string that uses pkgconfig.SSHSignatureHelperName), ensuring the removal code (the function containing that conditional) will correctly detect and remove entries if the helper name changes.
♻️ Duplicate comments (2)
pkg/loftconfig/config.go (1)
13-25:⚠️ Potential issue | 🟡 MinorUse dynamic CLI name in failure/debug messages too.
Line 14 executes
pkgconfig.BinaryName, but Lines 23 and 25 still hard-codedevpod pro login, which can mislead users in renamed builds.Suggested patch
func AuthDevpodCliToPlatform(config *client.Config, logger log.Logger) error { + loginCmd := fmt.Sprintf("%s pro login", pkgconfig.BinaryName) cmd := exec.Command( // `#nosec` G204 -- binary name is a compile-time constant pkgconfig.BinaryName, "pro", "login", @@ out, err := cmd.CombinedOutput() if err != nil { - logger.Debugf("Failed executing `devpod pro login`: %w, output: %s", err, out) + logger.Debugf("Failed executing `%s`: %w, output: %s", loginCmd, err, out) return fmt.Errorf( - "error executing 'devpod pro login' command: %w, access key: %v, host: %v", + "error executing '%s' command: %w, access key: %v, host: %v", + loginCmd, err, config.AccessKey, config.Host, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/loftconfig/config.go` around lines 13 - 25, The debug and error messages hard-code "devpod pro login" while the command uses pkgconfig.BinaryName; update the messages in the error path (the logger.Debugf call and the fmt.Errorf construction) to reference the actual binary name dynamically (e.g., construct the command name using pkgconfig.BinaryName plus the "pro login" args) instead of the literal string so logs reflect the real CLI name; adjust logger.Debugf and fmt.Errorf to include that constructed command identifier and keep existing error and output parameters (references: pkgconfig.BinaryName, logger.Debugf, fmt.Errorf).pkg/devcontainer/config/result.go (1)
10-10:⚠️ Potential issue | 🟠 MajorKeep legacy
devpodkey/label reads to avoid silent regressions.This still makes persisted keys branding-dependent. Existing
customizations.devpodanddevpod.userdata can stop being recognized after aBinaryNamechange.🔁 Compatibility-safe fallback patch
-const UserLabel = pkgconfig.BinaryName + ".user" +const ( + LegacyCustomizationKey = "devpod" + LegacyUserLabel = "devpod.user" + UserLabel = pkgconfig.BinaryName + ".user" +) func GetDevPodCustomizations(parsedConfig *DevContainerConfig) *DevPodCustomizations { - if parsedConfig.Customizations == nil || - parsedConfig.Customizations[pkgconfig.BinaryName] == nil { + if parsedConfig.Customizations == nil { return &DevPodCustomizations{} } + raw := parsedConfig.Customizations[pkgconfig.BinaryName] + if raw == nil { + raw = parsedConfig.Customizations[LegacyCustomizationKey] + } + if raw == nil { + return &DevPodCustomizations{} + } devPod := &DevPodCustomizations{} - err := Convert(parsedConfig.Customizations[pkgconfig.BinaryName], devPod) + err := Convert(raw, devPod) if err != nil { return &DevPodCustomizations{} }Also applies to: 71-73, 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/devcontainer/config/result.go` at line 10, Define and preserve legacy label keys alongside the new branding-based ones: add a LegacyUserLabel constant set to "devpod.user" (and a LegacyCustomizationsKey "customizations.devpod" for the nearby constants) and update any lookup code that reads labels or customizations to try the current UserLabel/CustomizationsKey first and fall back to LegacyUserLabel/LegacyCustomizationsKey; leave writes using the current pkgconfig.BinaryName-based constants but ensure readers accept both so existing persisted data isn't ignored.
🧹 Nitpick comments (7)
cmd/agent/container/setup.go (1)
350-354: LGTM! Daemon name correctly centralized.The change to use
config2.BinaryName + ".daemon"aligns with the PR's goal of consolidating repository variables.Minor observation: The log message on line 352 still contains hardcoded "devpod" text. Consider using a centralized constant for consistency if this is a broader refactoring goal, though this is purely cosmetic and doesn't affect functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/agent/container/setup.go` around lines 350 - 354, The debug log still uses the hardcoded string "devpod" — update the logger.Debugf in the StartBackgroundOnce block (the call near command.StartBackgroundOnce and config2.BinaryName+".daemon") to use the centralized binary name instead (e.g., use config2.BinaryName or a shared constant) so the message reads like "start <binary>.container daemon with inactivity timeout %s" and keep the workspaceInfo.ContainerTimeout argument unchanged.pkg/driver/custom/custom.go (1)
316-322: Minor inconsistency: Consider usingpkgconfig.BoolTruefor the debug value.Line 321 uses the literal
"true"while line 290 usespkgconfig.BoolTruefor the same purpose. For consistency with the centralization effort, consider:♻️ Suggested refactor for consistency
// set debug level if log.GetLevel() == logrus.DebugLevel { - environ = append(environ, pkgconfig.EnvDebug+"=true") + environ = append(environ, pkgconfig.EnvDebug+"="+pkgconfig.BoolTrue) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/driver/custom/custom.go` around lines 316 - 322, Replace the literal "true" with the centralized constant by changing the environment append when debug is enabled: in the block that checks log.GetLevel() == logrus.DebugLevel and appends pkgconfig.EnvDebug, use pkgconfig.BoolTrue instead of the string "true" so it matches the earlier usage pattern (see the environ append logic and pkgconfig.EnvDebug reference).desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx (1)
561-563: Consider usingstartsWithfor precise prefix removal.
String.replace()removes the first occurrence anywhere in the string, not just at the beginning. If a repo name unexpectedly containsPROVIDER_PREFIXmid-string, this would produce incorrect results.This is a low-risk edge case given typical repo naming conventions, but a more precise approach would be:
♻️ Suggested improvement
function stripDevpodPrefix(rawCommunityProvider: string): string { - return rawCommunityProvider.replace(PROVIDER_PREFIX, "") + return rawCommunityProvider.startsWith(PROVIDER_PREFIX) + ? rawCommunityProvider.slice(PROVIDER_PREFIX.length) + : rawCommunityProvider }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx` around lines 561 - 563, The stripDevpodPrefix function currently uses String.replace which can remove the PROVIDER_PREFIX anywhere in the string; change it to check if rawCommunityProvider.startsWith(PROVIDER_PREFIX) and only then remove the prefix (e.g., via slice or substring), otherwise return the original string unchanged; update stripDevpodPrefix to use startsWith and slice so only a leading PROVIDER_PREFIX is stripped.desktop/src/contexts/DevPodContext/action/actionHistory.ts (1)
2-4: Consider migrating existing action history data.Changing the localStorage key will cause existing users to lose their workspace action history (up to 50 entries). While this isn't critical data, it may be unexpected. Consider adding a one-time migration in the constructor:
💡 Optional migration helper
const HISTORY_KEY = `${BINARY_NAME}-workspace-action-history` +const LEGACY_HISTORY_KEY = "devpod-workspace-action-history" const MAX_HISTORY_ENTRIES = 50 export class ActionHistory { private active = new Map<string, Action>() private history: TActionObj[] private localStorageKey: string constructor(keySuffix?: string) { let localStorageKey = HISTORY_KEY if (keySuffix) { localStorageKey = `${localStorageKey}-${keySuffix}` } this.localStorageKey = localStorageKey - const maybeHistory = localStorage.getItem(this.localStorageKey) + let maybeHistory = localStorage.getItem(this.localStorageKey) + // Migrate from legacy key if new key doesn't exist + if (maybeHistory === null) { + const legacyKey = keySuffix ? `${LEGACY_HISTORY_KEY}-${keySuffix}` : LEGACY_HISTORY_KEY + maybeHistory = localStorage.getItem(legacyKey) + if (maybeHistory !== null) { + localStorage.setItem(this.localStorageKey, maybeHistory) + localStorage.removeItem(legacyKey) + } + } if (maybeHistory === null) { this.history = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@desktop/src/contexts/DevPodContext/action/actionHistory.ts` around lines 2 - 4, The change to HISTORY_KEY will orphan existing localStorage data; add a one-time migration in the module initialization (or constructor) to detect and move old data into the new key: locate HISTORY_KEY and BINARY_NAME in actionHistory.ts, detect the previous key(s) (e.g., the hardcoded old key string or prior BINARY_NAME value), read and validate its stored array (up to 50 entries), write it to the new HISTORY_KEY and remove the old key, and ensure this runs before any reads/writes of the action history so existing users keep their entries.pkg/provider/env.go (1)
72-78: Consider simplifying the double assignment.The folder path is assigned and then immediately normalized. This could be combined into a single statement for clarity:
♻️ Proposed simplification
- retVars[config.EnvProviderWorkspaceFolder], _ = GetWorkspaceDir( - workspace.Context, - workspace.ID, - ) - retVars[config.EnvProviderWorkspaceFolder] = filepath.ToSlash( - retVars[config.EnvProviderWorkspaceFolder], - ) + workspaceDir, _ := GetWorkspaceDir(workspace.Context, workspace.ID) + retVars[config.EnvProviderWorkspaceFolder] = filepath.ToSlash(workspaceDir)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provider/env.go` around lines 72 - 78, The code currently assigns retVars[config.EnvProviderWorkspaceFolder] from GetWorkspaceDir(workspace.Context, workspace.ID) then immediately reassigns it to filepath.ToSlash(...); combine these into a single assignment by calling filepath.ToSlash on the GetWorkspaceDir return value so retVars[config.EnvProviderWorkspaceFolder] is set once; update the lines referencing GetWorkspaceDir, filepath.ToSlash, retVars and config.EnvProviderWorkspaceFolder accordingly to remove the intermediate assignment.pkg/client/clientimplementation/workspace_client.go (1)
797-797: Useconfig.BoolTruein debug env assignment too.Line 797 still embeds
"true"directly; using the centralized bool constant keeps the migration complete.Proposed refactor
- opts.Environ = append(opts.Environ, config.EnvDebug+"=true") + opts.Environ = append(opts.Environ, config.EnvDebug+"="+config.BoolTrue)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/client/clientimplementation/workspace_client.go` at line 797, Replace the hard-coded "true" string in the environment assignment with the centralized boolean constant: change the append call that builds opts.Environ using config.EnvDebug to use config.BoolTrue instead of the literal "true" (i.e., update the expression that currently does opts.Environ = append(opts.Environ, config.EnvDebug+"=true") to use config.BoolTrue so it becomes config.EnvDebug+"="+config.BoolTrue); this ensures the debug env flag uses the shared constant.cmd/pro/provider/up.go (1)
115-116: Preferconfig.EnvDebugover literal"DEBUG"for consistency.Line 115 hard-codes the env key while Line 40 already uses the centralized constant.
Proposed refactor
- if options != nil && os.Getenv("DEBUG") == config.BoolTrue { + if options != nil && os.Getenv(config.EnvDebug) == config.BoolTrue { options.Add("debug", config.BoolTrue) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pro/provider/up.go` around lines 115 - 116, Replace the hard-coded environment key "DEBUG" with the centralized constant config.EnvDebug in the conditional that checks options and debug; specifically update the os.Getenv call used alongside options and config.BoolTrue (the block that calls options.Add("debug", config.BoolTrue)) to use config.EnvDebug so it matches the earlier usage and maintains consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/pro/provider/get/version.go`:
- Around line 70-73: The error uses the empty value of providerID in the
message, producing "provider ID not defined"; update the check around
providerID (the code that reads os.Getenv(config.EnvProviderID)) to include the
environment variable key (config.EnvProviderID) in the returned error instead of
the empty providerID value so the error becomes actionable and references the
required env var name.
In `@desktop/src/components/useInstallCLI.tsx`:
- Line 119: The help text in useInstallCLI.tsx currently shows the incorrect
Windows env var string "%APP_DATA%\\${FLATPAK_ID}\\bin"; update the displayed
string to use the correct Windows variable "%APPDATA%\\${FLATPAK_ID}\\bin"
inside the <Code> element or template literal (where the message is constructed)
so users see the proper %APPDATA% spelling.
---
Outside diff comments:
In `@desktop/src/views/Pro/Workspace/WorkspaceDetails.tsx`:
- Around line 440-443: The code still uses the hard-coded string "devpod" when
locating a container status; update the lookup to use the centralized container
name value instead (e.g., reference the PodStatus/containerName constant or
accessor you added) so containerStatus =
podStatus.containerStatuses?.find(container => container.name ===
<centralizedContainerName> && (container.state?.waiting?.reason ||
container.state?.terminated?.reason)); ensure you import or reference the same
symbol used elsewhere in PodStatus (the centralized container name or getter)
and replace the literal in WorkspaceDetails.tsx so resolution works for
different runtime container names.
In `@pkg/gitsshsigning/helper.go`:
- Line 213: The removal logic is using a hardcoded string "program =
devpod-ssh-signature" instead of the constant; update the check to use
pkgconfig.SSHSignatureHelperName so the match follows the canonical helper name
used elsewhere (replace the literal in the conditional that currently reads
"program = devpod-ssh-signature" with a formatted string that uses
pkgconfig.SSHSignatureHelperName), ensuring the removal code (the function
containing that conditional) will correctly detect and remove entries if the
helper name changes.
In `@pkg/loftconfig/config.go`:
- Around line 24-28: The error message built with fmt.Errorf currently includes
config.AccessKey which leaks secrets; remove the AccessKey from the error
payload and stop returning it (for example, drop config.AccessKey from the
fmt.Errorf call in the code that constructs "error executing 'devpod pro login'
command"), leaving only the error and non-secret fields (e.g., err and
config.Host) or replace the key with a redacted placeholder if you must indicate
presence, and ensure any related error formatting sites (the fmt.Errorf call) no
longer reference config.AccessKey.
---
Duplicate comments:
In `@pkg/devcontainer/config/result.go`:
- Line 10: Define and preserve legacy label keys alongside the new
branding-based ones: add a LegacyUserLabel constant set to "devpod.user" (and a
LegacyCustomizationsKey "customizations.devpod" for the nearby constants) and
update any lookup code that reads labels or customizations to try the current
UserLabel/CustomizationsKey first and fall back to
LegacyUserLabel/LegacyCustomizationsKey; leave writes using the current
pkgconfig.BinaryName-based constants but ensure readers accept both so existing
persisted data isn't ignored.
In `@pkg/loftconfig/config.go`:
- Around line 13-25: The debug and error messages hard-code "devpod pro login"
while the command uses pkgconfig.BinaryName; update the messages in the error
path (the logger.Debugf call and the fmt.Errorf construction) to reference the
actual binary name dynamically (e.g., construct the command name using
pkgconfig.BinaryName plus the "pro login" args) instead of the literal string so
logs reflect the real CLI name; adjust logger.Debugf and fmt.Errorf to include
that constructed command identifier and keep existing error and output
parameters (references: pkgconfig.BinaryName, logger.Debugf, fmt.Errorf).
---
Nitpick comments:
In `@cmd/agent/container/setup.go`:
- Around line 350-354: The debug log still uses the hardcoded string "devpod" —
update the logger.Debugf in the StartBackgroundOnce block (the call near
command.StartBackgroundOnce and config2.BinaryName+".daemon") to use the
centralized binary name instead (e.g., use config2.BinaryName or a shared
constant) so the message reads like "start <binary>.container daemon with
inactivity timeout %s" and keep the workspaceInfo.ContainerTimeout argument
unchanged.
In `@cmd/pro/provider/up.go`:
- Around line 115-116: Replace the hard-coded environment key "DEBUG" with the
centralized constant config.EnvDebug in the conditional that checks options and
debug; specifically update the os.Getenv call used alongside options and
config.BoolTrue (the block that calls options.Add("debug", config.BoolTrue)) to
use config.EnvDebug so it matches the earlier usage and maintains consistency.
In `@desktop/src/contexts/DevPodContext/action/actionHistory.ts`:
- Around line 2-4: The change to HISTORY_KEY will orphan existing localStorage
data; add a one-time migration in the module initialization (or constructor) to
detect and move old data into the new key: locate HISTORY_KEY and BINARY_NAME in
actionHistory.ts, detect the previous key(s) (e.g., the hardcoded old key string
or prior BINARY_NAME value), read and validate its stored array (up to 50
entries), write it to the new HISTORY_KEY and remove the old key, and ensure
this runs before any reads/writes of the action history so existing users keep
their entries.
In `@desktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsx`:
- Around line 561-563: The stripDevpodPrefix function currently uses
String.replace which can remove the PROVIDER_PREFIX anywhere in the string;
change it to check if rawCommunityProvider.startsWith(PROVIDER_PREFIX) and only
then remove the prefix (e.g., via slice or substring), otherwise return the
original string unchanged; update stripDevpodPrefix to use startsWith and slice
so only a leading PROVIDER_PREFIX is stripped.
In `@pkg/client/clientimplementation/workspace_client.go`:
- Line 797: Replace the hard-coded "true" string in the environment assignment
with the centralized boolean constant: change the append call that builds
opts.Environ using config.EnvDebug to use config.BoolTrue instead of the literal
"true" (i.e., update the expression that currently does opts.Environ =
append(opts.Environ, config.EnvDebug+"=true") to use config.BoolTrue so it
becomes config.EnvDebug+"="+config.BoolTrue); this ensures the debug env flag
uses the shared constant.
In `@pkg/driver/custom/custom.go`:
- Around line 316-322: Replace the literal "true" with the centralized constant
by changing the environment append when debug is enabled: in the block that
checks log.GetLevel() == logrus.DebugLevel and appends pkgconfig.EnvDebug, use
pkgconfig.BoolTrue instead of the string "true" so it matches the earlier usage
pattern (see the environ append logic and pkgconfig.EnvDebug reference).
In `@pkg/provider/env.go`:
- Around line 72-78: The code currently assigns
retVars[config.EnvProviderWorkspaceFolder] from
GetWorkspaceDir(workspace.Context, workspace.ID) then immediately reassigns it
to filepath.ToSlash(...); combine these into a single assignment by calling
filepath.ToSlash on the GetWorkspaceDir return value so
retVars[config.EnvProviderWorkspaceFolder] is set once; update the lines
referencing GetWorkspaceDir, filepath.ToSlash, retVars and
config.EnvProviderWorkspaceFolder accordingly to remove the intermediate
assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7fc8f47-38d9-415c-9c44-78d3b56775ec
📒 Files selected for processing (120)
cmd/agent/agent.gocmd/agent/container/daemon.gocmd/agent/container/setup.gocmd/agent/container/ssh_server.gocmd/agent/container_tunnel.gocmd/agent/workspace/up.gocmd/build.gocmd/delete.gocmd/flags/flags.gocmd/helper/get_workspace_config.gocmd/ide/use.gocmd/pro/daemon/start.gocmd/pro/login.gocmd/pro/pro.gocmd/pro/provider/create/workspace.gocmd/pro/provider/get/version.gocmd/pro/provider/list/workspaces.gocmd/pro/provider/provider.gocmd/pro/provider/ssh.gocmd/pro/provider/status.gocmd/pro/provider/up.gocmd/pro/provider/watch/workspaces.gocmd/pro/start.gocmd/pro/version.gocmd/pro/watch_workspaces.gocmd/provider/list-default-providers.gocmd/provider/set_options.gocmd/provider/update.gocmd/provider/use.gocmd/root.gocmd/ssh.gocmd/status.gocmd/up.godesktop/src/App/useChangelogModal.tsxdesktop/src/client/client.tsdesktop/src/client/command.tsdesktop/src/client/constants.tsdesktop/src/client/repo.tsdesktop/src/components/Layout/ProSwitcher.tsxdesktop/src/components/Layout/StatusBar.tsxdesktop/src/components/LoftOSSBadge.tsxdesktop/src/components/useInstallCLI.tsxdesktop/src/contexts/DevPodContext/Pro/ProProvider.tsxdesktop/src/contexts/DevPodContext/action/actionHistory.tsdesktop/src/contexts/DevPodContext/proInstances/useProInstanceManager.tsdesktop/src/lib/store.tsdesktop/src/lib/useStoreTroubleshoot.tsdesktop/src/main.tsxdesktop/src/useWelcomeModal.tsxdesktop/src/views/Pro/CreateWorkspace/IDEInput.tsxdesktop/src/views/Pro/Workspace/WorkspaceDetails.tsxdesktop/src/views/Providers/AddProvider/SetupProviderSourceForm.tsxdesktop/src/views/Settings/useContextOptions.tsxdesktop/src/views/Workspaces/CreateWorkspace/CreateWorkspace.tsxdesktop/src/views/Workspaces/WorkspaceControls.tsxhack/pro/main.gopkg/agent/agent.gopkg/agent/agent_test.gopkg/agent/binary.gopkg/agent/inject.gopkg/agent/tunnelserver/tunnelserver.gopkg/agent/workspace.gopkg/client/clientimplementation/daemonclient/client.gopkg/client/clientimplementation/daemonclient/up.gopkg/client/clientimplementation/machine_client.gopkg/client/clientimplementation/proxy_client.gopkg/client/clientimplementation/services.gopkg/client/clientimplementation/workspace_client.gopkg/config/dir.gopkg/config/env.gopkg/config/paths.gopkg/config/product.gopkg/config/repo.gopkg/credentials/server.gopkg/daemon/agent/daemon.gopkg/daemon/platform/daemon.gopkg/daemon/platform/socket.gopkg/devcontainer/build.gopkg/devcontainer/config.gopkg/devcontainer/config/build.gopkg/devcontainer/config/prebuild.gopkg/devcontainer/config/result.gopkg/devcontainer/crane/run.gopkg/devcontainer/feature/features.gopkg/devcontainer/setup/setup.gopkg/devcontainer/single.gopkg/dockercredentials/dockercredentials.gopkg/dockercredentials/helper.gopkg/driver/custom/custom.gopkg/driver/docker/docker.gopkg/driver/kubernetes/driver.gopkg/driver/kubernetes/init_container.gopkg/driver/kubernetes/run.gopkg/gitcredentials/gitcredentials.gopkg/gitsshsigning/helper.gopkg/ide/fleet/fleet.gopkg/ide/ideparse/parse.gopkg/ide/jetbrains/generic.gopkg/ide/rstudio/rstudio.gopkg/ide/vscode/open.gopkg/ide/zed/zed.gopkg/ide/zed/zed_linux.gopkg/inject/download_urls.gopkg/inject/inject.gopkg/loftconfig/config.gopkg/options/resolve.gopkg/platform/client/client.gopkg/platform/deploy.gopkg/provider/dir.gopkg/provider/download.gopkg/provider/env.gopkg/ssh/config.gopkg/ssh/keys.gopkg/telemetry/collect.gopkg/ts/util.gopkg/tunnel/services.gopkg/upgrade/upgrade.gopkg/workspace/list.gopkg/workspace/machine.gopkg/workspace/provider.go
💤 Files with no reviewable changes (1)
- pkg/client/clientimplementation/daemonclient/client.go
✅ Files skipped from review due to trivial changes (21)
- cmd/provider/update.go
- pkg/agent/tunnelserver/tunnelserver.go
- pkg/ts/util.go
- desktop/src/components/Layout/ProSwitcher.tsx
- desktop/src/components/LoftOSSBadge.tsx
- pkg/driver/docker/docker.go
- pkg/driver/kubernetes/driver.go
- pkg/ide/zed/zed_linux.go
- pkg/config/product.go
- pkg/daemon/agent/daemon.go
- pkg/platform/client/client.go
- cmd/helper/get_workspace_config.go
- pkg/config/paths.go
- pkg/client/clientimplementation/daemonclient/up.go
- pkg/ide/rstudio/rstudio.go
- pkg/agent/workspace.go
- pkg/provider/dir.go
- pkg/config/repo.go
- desktop/src/client/repo.ts
- pkg/ide/ideparse/parse.go
- pkg/config/env.go
🚧 Files skipped from review as they are similar to previous changes (29)
- cmd/flags/flags.go
- pkg/daemon/platform/socket.go
- pkg/upgrade/upgrade.go
- cmd/pro/pro.go
- cmd/build.go
- cmd/provider/list-default-providers.go
- pkg/devcontainer/feature/features.go
- pkg/workspace/machine.go
- pkg/daemon/platform/daemon.go
- hack/pro/main.go
- cmd/pro/daemon/start.go
- pkg/devcontainer/config/prebuild.go
- cmd/provider/set_options.go
- cmd/pro/provider/provider.go
- pkg/ide/jetbrains/generic.go
- pkg/devcontainer/crane/run.go
- cmd/root.go
- pkg/ide/vscode/open.go
- pkg/gitcredentials/gitcredentials.go
- cmd/pro/login.go
- cmd/agent/container/ssh_server.go
- pkg/inject/download_urls.go
- pkg/workspace/provider.go
- cmd/agent/container/daemon.go
- pkg/agent/binary.go
- pkg/ssh/keys.go
- pkg/config/dir.go
- cmd/agent/workspace/up.go
- pkg/devcontainer/setup/setup.go
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
skevetter
force-pushed
the
centralize-repo-config
branch
from
3cb570b to
6d4691a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Samuel K skevetter@pm.me
Summary by CodeRabbit