feat: probe host ports before starting the dev environment - #1323
Draft
Soner (shyim) wants to merge 6 commits into
Draft
feat: probe host ports before starting the dev environment#1323Soner (shyim) wants to merge 6 commits into
Soner (shyim) wants to merge 6 commits into
Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true📝 WalkthroughWalkthroughThe change adds Docker host-port configuration, local override persistence, conflict detection, random port allocation, Compose regeneration, and CLI/TUI handling before development and migration startup. ChangesDocker port management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant DevelopmentCLI
participant DockerPortScanner
participant PortAllocator
participant ComposeGenerator
participant LocalConfig
Developer->>DevelopmentCLI: start development environment
DevelopmentCLI->>DockerPortScanner: find port conflicts
DockerPortScanner-->>DevelopmentCLI: return conflicts
DevelopmentCLI->>PortAllocator: allocate random replacements
PortAllocator-->>DevelopmentCLI: return port map
DevelopmentCLI->>ComposeGenerator: regenerate compose.yaml
DevelopmentCLI->>LocalConfig: persist Docker port overrides
DevelopmentCLI-->>Developer: start containers or report conflicts
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
internal/shop/config_local_test.go (1)
79-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
t.Context()instead ofcontext.Background().The coding guidelines require
t.Context()for test contexts. The sibling fileinternal/shop/config_test.goalready follows this. This file usescontext.Background()at lines 86, 98, 115, 202, and 234.After replacing all five call sites, remove the now-unused
contextimport at line 4.♻️ Proposed change for the two call sites in this range
- cfg, err := ReadConfig(context.Background(), configPath, false) + cfg, err := ReadConfig(t.Context(), configPath, false) require.NoError(t, err) require.NotNil(t, cfg.Docker) assert.Equal(t, ConfigDockerPort(52341), cfg.Docker.Ports[DockerPortWeb])- cfg, err := ReadConfig(context.Background(), configPath, true) + cfg, err := ReadConfig(t.Context(), configPath, true)As per coding guidelines: "use
t.Context()for test contexts".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/shop/config_local_test.go` around lines 79 - 118, Replace every context.Background() call in the tests of this file, including the calls in TestUpdateLocalDockerPortsRoundTripsThroughReadConfig and TestReadConfigMergesLocalFileWithoutBaseConfig, with the corresponding test’s t.Context(). Apply the same change to the other three call sites, then remove the unused context import.Source: Coding guidelines
internal/docker/compose_test.go (1)
164-169: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider parsing the YAML instead of slicing the compose text.
strings.Cut(compose, "adminer:")matches the first occurrence of the literaladminer:. If a future compose template emitsadminer:earlier, for example as a mapping key inside adepends_onblock on thewebservice, the slice would cover the wrong region and the assertion would pass without testing the adminer service.Unmarshaling the generated file and reading
services.adminer.portskeeps the assertion tied to the intended node.♻️ Proposed change
- // A service whose ports are all disabled loses its ports key entirely. - _, adminerSection, found := strings.Cut(compose, "adminer:") - assert.True(t, found) - adminerSection, _, found = strings.Cut(adminerSection, "mailer:") - assert.True(t, found) - assert.NotContains(t, adminerSection, "ports:") + // A service whose ports are all disabled loses its ports key entirely. + var parsed struct { + Services map[string]struct { + Ports []string `yaml:"ports"` + } `yaml:"services"` + } + assert.NoError(t, yaml.Unmarshal(result, &parsed)) + assert.Empty(t, parsed.Services["adminer"].Ports)This also removes the need for the
stringsimport if no other assertion uses it.Run the following script to confirm the current compose layout and whether
adminer:can appear before the service definition:#!/bin/bash # Description: Inspect the compose generation template for depends_on usage and service ordering. set -euo pipefail fd -t f 'compose' internal/docker # Map the compose generator. ast-grep outline internal/docker/compose.go --items all # Look for depends_on rendering and the adminer service block. rg -n -C 6 'depends_on|adminer' internal/docker --type=go -g '!*_test.go' # Check for an embedded compose template file. fd -e tmpl -e yaml -e yml . internal/docker --exec rg -n -C 4 'adminer|depends_on' {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/docker/compose_test.go` around lines 164 - 169, Replace the text slicing around the generated compose output in the relevant test with YAML unmarshaling, then inspect the parsed services.adminer node and assert that its ports field is absent. Remove the strings import if it is no longer used, while preserving the existing validation that the adminer service exists.cmd/project/project_dev.go (1)
222-228: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
e.configPathfor consistency.Line 227 reads the package-level
projectConfigPath, while lines 243 and 250 in the same method reade.configPath. Both hold the same value today, because line 194 assignsprojectConfigPathto the field. Reading one source in the whole method prevents the error message from pointing at a different file than the one the method writes.♻️ Proposed change
- return fmt.Errorf("cannot start the development environment, host ports are already in use:\n%s\nrerun with --on-port-conflict=random to switch them to free ports, or set docker.ports in %s", strings.Join(lines, "\n"), projectConfigPath) + return fmt.Errorf("cannot start the development environment, host ports are already in use:\n%s\nrerun with --on-port-conflict=random to switch them to free ports, or set docker.ports in %s", strings.Join(lines, "\n"), e.configPath)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/project/project_dev.go` around lines 222 - 228, Update the error message in the port-conflict handling branch of the enclosing method to use the method’s existing e.configPath field instead of the package-level projectConfigPath variable, matching the references used by the later branches.internal/tui/dev/lifecycle_test.go (1)
358-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
require.NoErrorin the setup helper.
writeMinimalComposeProjectestablishes a precondition. If thecomposer.lockwrite fails,assert.NoErrorrecords the failure but lets the test continue.fixPortConflictsthen fails for a second, unrelated reason, which hides the root cause.♻️ Proposed change
func writeMinimalComposeProject(t *testing.T, dir string) { t.Helper() lock := `{"packages": [{"name": "shopware/core", "version": "6.6.0.0"}], "packages-dev": []}` - assert.NoError(t, os.WriteFile(filepath.Join(dir, "composer.lock"), []byte(lock), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "composer.lock"), []byte(lock), 0o644)) }Add
"github.com/stretchr/testify/require"to the import block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/dev/lifecycle_test.go` around lines 358 - 363, Update writeMinimalComposeProject to use require.NoError for the os.WriteFile result, and add the testify/require import so setup failures stop the test immediately before dependent logic runs.internal/shop/config_local.go (1)
109-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the credential key list from one source.
The credential keys appear twice: in the
secretsmap literal and in the loop slice. A future key must be added in both places, otherwise it is written but never cleared.♻️ Proposed dedupe
- secrets := map[string]string{ - "blackfire_server_id": "", - "blackfire_server_token": "", - "tideways_api_key": "", - } - if php != nil { - secrets["blackfire_server_id"] = php.BlackfireServerID - secrets["blackfire_server_token"] = php.BlackfireServerToken - secrets["tideways_api_key"] = php.TidewaysAPIKey - } + type credential struct { + key string + value string + } + credentials := []credential{ + {key: "blackfire_server_id"}, + {key: "blackfire_server_token"}, + {key: "tideways_api_key"}, + } + if php != nil { + credentials[0].value = php.BlackfireServerID + credentials[1].value = php.BlackfireServerToken + credentials[2].value = php.TidewaysAPIKey + }Then iterate
credentialsin the mutate function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/shop/config_local.go` around lines 109 - 142, Define a single credential key list, such as credentials, and use it to initialize or populate the secrets map and to drive the mutation loop in updateLocalConfig. Remove the duplicated inline key slice while preserving the existing set-or-delete behavior for each credential.internal/docker/ports_test.go (1)
63-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
busyto match what it returns.
busyreturns anisFreepredicate. The callbusy(8000)reads as "8000 is busy", but the returned function reportsfalsefor 8000. The inversion inside the closure makes the helper harder to follow.♻️ Proposed rename
- busy := func(ports ...int) func(int) bool { + // freeExcept returns an isFree predicate that reports the given ports as busy. + freeExcept := func(ports ...int) func(int) bool {Update the call sites accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/docker/ports_test.go` around lines 63 - 72, Rename the test helper busy to isFree to reflect that its returned predicate reports whether a port is available, and update every call site accordingly. Preserve the existing predicate behavior and port-set logic while removing the misleading busy naming.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/docker/ports.go`:
- Around line 156-163: Update ownPublishedPorts to determine the Compose project
name from the inherited COMPOSE_PROJECT_NAME or
shop.ReadComposeProjectName(projectFolder) and pass it to docker compose ps via
-p. Also update the full FindPortConflicts probe, including ownPublishedPorts
and port checks, to run under a bounded timeout even when called with
context.Background().
In `@internal/shop/config_local.go`:
- Around line 53-63: Update the local configuration write flow around
os.WriteFile to write the secret content to a same-directory temporary file with
mode 0600, then atomically rename it over localFile. Add the required filepath
handling, ensure temporary files are cleaned up on failure, and preserve the
existing wrapped errors for write and rename failures.
In `@internal/shop/config_override.go`:
- Around line 220-238: Update normalizeTimestampValue for time.Time values to
determine date-only status from the value’s local wall-clock fields, replacing
the v.Truncate comparison. Only format as time.DateOnly when the local hour,
minute, second, and nanosecond are all zero; otherwise preserve the RFC3339
formatting.
In `@internal/tui/dev/lifecycle_test.go`:
- Around line 341-344: Update the persistence assertion near the local override
file read in the lifecycle test to verify the allocated web port value from
result.overrides[shop.DockerPortWeb], rather than checking only for the
ambiguous "web:" key. Keep the existing file-read and error assertion, and
ensure the persisted content must contain the expected port value.
In `@internal/tui/dev/lifecycle.go`:
- Around line 128-131: Guard m.config before calling SetDockerPortOverrides in
the update path around startContainers: ensure a nil config is initialized or
handled safely before applying non-empty msg.overrides, matching the nil-config
behavior permitted by fixPortConflicts. Preserve the existing override
application and container-start flow when m.config is available.
---
Nitpick comments:
In `@cmd/project/project_dev.go`:
- Around line 222-228: Update the error message in the port-conflict handling
branch of the enclosing method to use the method’s existing e.configPath field
instead of the package-level projectConfigPath variable, matching the references
used by the later branches.
In `@internal/docker/compose_test.go`:
- Around line 164-169: Replace the text slicing around the generated compose
output in the relevant test with YAML unmarshaling, then inspect the parsed
services.adminer node and assert that its ports field is absent. Remove the
strings import if it is no longer used, while preserving the existing validation
that the adminer service exists.
In `@internal/docker/ports_test.go`:
- Around line 63-72: Rename the test helper busy to isFree to reflect that its
returned predicate reports whether a port is available, and update every call
site accordingly. Preserve the existing predicate behavior and port-set logic
while removing the misleading busy naming.
In `@internal/shop/config_local_test.go`:
- Around line 79-118: Replace every context.Background() call in the tests of
this file, including the calls in
TestUpdateLocalDockerPortsRoundTripsThroughReadConfig and
TestReadConfigMergesLocalFileWithoutBaseConfig, with the corresponding test’s
t.Context(). Apply the same change to the other three call sites, then remove
the unused context import.
In `@internal/shop/config_local.go`:
- Around line 109-142: Define a single credential key list, such as credentials,
and use it to initialize or populate the secrets map and to drive the mutation
loop in updateLocalConfig. Remove the duplicated inline key slice while
preserving the existing set-or-delete behavior for each credential.
In `@internal/tui/dev/lifecycle_test.go`:
- Around line 358-363: Update writeMinimalComposeProject to use require.NoError
for the os.WriteFile result, and add the testify/require import so setup
failures stop the test immediately before dependent logic runs.
🪄 Autofix
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 Plus
Run ID: 58406773-933a-4217-8b67-08e4eaf65d94
📒 Files selected for processing (21)
cmd/project/project_dev.gointernal/docker/compose.gointernal/docker/compose_test.gointernal/docker/ports.gointernal/docker/ports_test.gointernal/shop/config.gointernal/shop/config_docker_ports.gointernal/shop/config_local.gointernal/shop/config_local_test.gointernal/shop/config_override.gointernal/shop/config_schema.jsoninternal/tui/dev/lifecycle.gointernal/tui/dev/lifecycle_test.gointernal/tui/dev/model.gointernal/tui/dev/model_commands.gointernal/tui/dev/model_test.gointernal/tui/dev/model_update.gointernal/tui/dev/model_view.gointernal/tui/dev/overlay_port_conflict.gointernal/tui/dev/tab_overview.gointernal/tui/dev/tab_overview_test.go
Contributor
|
Waiting until the local proxy is merged, then it needs a rebase |
Soner (shyim)
force-pushed
the
feat/dev-port-conflicts
branch
from
August 21, 2026 04:34
6ef77d0 to
eadc678
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1323 +/- ##
==========================================
+ Coverage 62.72% 63.31% +0.58%
==========================================
Files 435 455 +20
Lines 29476 30087 +611
==========================================
+ Hits 18490 19049 +559
- Misses 10986 11038 +52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Soner (shyim)
force-pushed
the
feat/dev-port-conflicts
branch
2 times, most recently
from
August 21, 2026 08:15
b075c23 to
b716a7e
Compare
Member
Author
|
i would wait on #1427 |
Soner (shyim)
force-pushed
the
feat/dev-port-conflicts
branch
from
August 31, 2026 08:08
1477488 to
da070ba
Compare
- Add docker.ports config (int|false) with machine-local overrides persisted to .shopware-project.local.yml (stripped from the committed config by WriteConfig) - Probe published host ports before starting, excluding own containers; new --on-port-conflict flag (fail|random) on project dev - Add Bubbletea port-conflict overlay in the dev TUI offering random re-allocation or quit - Skip probing for proxy-mode projects (no host ports published); keep probing after proxy fallback and write fixed-port compose in that case so the fallback is never silently undone
…earch deployment settings
Service, port, and URL knowledge was scattered across compose generation, port-conflict detection, proxy listing, and the dev TUI, so every new service meant touching half a dozen files. Introduce a single service catalog in internal/docker that defines each service's endpoints, default ports, proxy subdomains, credentials, and feature gate, and derive port definitions, proxy routes, access URLs, and the TUI overview from it. Environment discovery (docker compose ps parsing and container classification) moves from the TUI into the docker package next to the catalog, sharing one compose invocation and parser with port-conflict detection. The duplicated proxyHostname helpers are replaced by proxy.RegisteredHostname, and rustfs ports now participate in conflict detection and port overrides like every other service. Co-authored-by: Cursor <cursoragent@cursor.com>
Soner (shyim)
force-pushed
the
feat/dev-port-conflicts
branch
from
September 2, 2026 11:37
f532390 to
051aecd
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
3 participants
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.
What changed?
project devnow checks whether the host ports the generatedcompose.yamlwill publish are actually free before runningdocker compose up -d(ports already held by the project's own containers are excluded, so restarts never false-alarm).On conflict:
project dev): a modal lists the busy ports and offers Use random free ports / Quit.project dev start, orproject devwithout a TTY): new--on-port-conflict=fail|randomflag.fail(default) aborts with a descriptive error,randomremaps only the conflicting ports.Chosen ports are persisted per-key into
.shopware-project.local.yml(or.local.yaml) and reused on every later run. All published host ports are also directly configurable now, andfalsedisables publishing a port entirely:CLI output with port 8000 occupied (default
failmode):With
--on-port-conflict=random:Implementation notes:
internal/docker/ports.goholds the single source of truth for all 12 published ports (web 8000/8080/9999/9998/5173/5773, adminer, mailpit, conditional lavinmq/opensearch), the availability probe, and the random allocator (all listeners held open until every port is picked, so no duplicates).internal/shop/config_local.go) that preserves comments,${VAR}references, and unrelated keys. The TUI profiler-secret save was migrated onto it; the old full-file-rewriteWriteLocalConfig(which would have clobbered the ports) is removed.WriteConfigstripsdocker.portsso machine-local ports never leak into the committed config.ReadConfigignored.shopware-project.local.ymlentirely when the base config file is missing, and the local-override merge corrupted unquotedcompatibility_datevalues into RFC3339 timestamps (yaml.v3!!timestampresolution during themap[string]anyround-trip).Why?
The dev environment binds a lot of host ports and anything already listening (a second project, a stray
php -S, another mailpit) madedocker compose upfail with a raw error. Now conflicts are detected up front and can be resolved automatically, with the resolution remembered per machine without touching the committed config.How was this tested?
go test ./...andgolangci-lint run ./...are green;internal/shop/config_schema.jsonregenerated.ports:key dropped when a service has none left), local-overlay persistence (creates 0600 file, preserves comments/${VAR}/secrets,.yamlnaming,ReadConfiground-trip, unquoted-date regression), TUI lifecycle (conflict → prompt, quit, fix → start, fix error).failmode error above,randommode remap + persisted overlay + regenerated compose (60636:8000), rerun with 8000 still busy correctly finds no conflict (override is probed instead), andadminer: falseremoves theports:key from the service.Related issue or discussion
—
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
failandrandomconflict-handling modes.Bug Fixes