feat(open): add service URL opener - #552
Conversation
|
🚀 Website Preview
Preview has been cleaned up as the PR was closed. |
wbreza
left a comment
There was a problem hiding this comment.
Code Review — feat(open): add service URL opener
Review outcome: Comment. No Critical/High defects; two Medium and several Low findings. Approving is not warranted, but nothing here is a blocker.
What this PR does
Adds azd app open <service> — resolves a local service URL (running-app state first, then azure.yaml) and opens it in the browser, with --path to append a route and --print to emit the URL instead of launching. Clean, well-tested command with sensible flag seams (openURL override for tests).
Verification note
I built and type-checked the branch against the real packages. All referenced symbols resolve correctly (serviceinfo.LocalServiceInfo.CustomURL, service.Service.Local -> LocalServiceConfig.CustomURL, completeServiceArgs, github.com/pkg/browser). URL scheme safety is already enforced upstream: ParseAzureYaml rejects non-http(s) schemes, so file:/javascript: customUrls from azure.yaml cannot reach browser.OpenURL. No security finding.
Medium
1. go mod tidy is not committed — preflight/CI will fail.
github.com/pkg/browser is now used directly, but cli/go.mod still lists it as // indirect. Running go mod tidy under cli/ moves it to the direct require block and modifies go.mod. A CI "tidy" gate (or the repo's Preflight Checks) will flag this. Run go mod tidy in cli/ and commit the updated go.mod/go.sum.
2. Docker container-only ports resolve to the wrong URL.
hostPortFromMapping treats a bare port (e.g. "80") as the host port and emits http://localhost:80. For Docker services a bare port is the container port with an auto-assigned host port — the canonical service.ParsePortSpec returns HostPort: 0 for that case. The hand-rolled parser also skips the Docker/non-Docker distinction and does no numeric validation. Prefer reusing Service.GetPortMappings() / GetPrimaryPort() and only emit a URL when a real host port is published (HostPort > 0). See inline comment.
Low / Nits
- Swallowed
azure.yamlparse error (resolveOpenServiceURL):parseErris ignored, so a malformedazure.yamlsurfaces as "No services are defined" rather than the real error. Consider surfacing non-"missing file" errors. (inline) joinOpenURLPathmutatesPathwithout resettingRawPath: pre-encoded base paths (e.g..../a%2Fb) can be re-rendered as separate segments (.../a/b/...). Queries/fragments are preserved correctly. (inline)- Output channel:
--printusesfmt.Println, while sibling commands route throughinternal/output. Minor consistency nit. (inline) - Ignored
context.Context:resolveOpenServiceURLacceptsctxas_. Either thread it through the resolvers or drop the parameter. - Resolution flow: when a service is found in service-info but has no URL, the
breakfalls through to re-parseazure.yaml(whichGetServiceInfoalready parsed). ThefoundService"Start it with 'azd app run'" hint is hard to reach for a service defined inazure.yaml. Worth simplifying to a single source of truth. - Test gaps: no coverage for
--print, the not-found "Available services" list,hostPortFromMappingedge cases (3-partip:host:container,/tcpsuffix, bare port), or path-with-query. Add a table test forhostPortFromMappingin particular.
Nice, focused feature — the two Mediums (tidy + docker bare-port) are the ones worth addressing before merge.
|
@wbreza thanks — genuinely useful review, and the two Mediums were both real. All four inline threads have replies with commit links; below are the items that only appeared in the review body. Commits:
Confirmed — Preflight was already red on exactly this: Fixed in
Dropped it in
Partially agreed, and restructured in I checked What was genuinely broken, and is now fixed:
All added in
One correction, detailed in the Local verification: Edited: the branch was rebased onto main to resolve a cli/go.mod conflict with a prometheus/client_golang bump that landed on main. Commit links above point at the post-rebase SHAs. Resolution took main's dependency set plus the direct pkg/browser entry; go mod tidy, go build ./..., the full commands package tests and golangci-lint were all re-run green on the rebased tree. |
wbreza
left a comment
There was a problem hiding this comment.
Code Review (re-review) — feat(open): add service URL opener
Review outcome: Approve. All findings from my previous review have been resolved, and the three follow-up commits introduce no new Critical/High/Medium issues. Verified by building and running the tests against the new head.
Verification
I built the commands package and ran the suite in an isolated worktree at ac7154e:
go build ./src/cmd/app/commands/— passgo vet ./src/cmd/app/commands/— passgo test ./src/cmd/app/commands/ -run Open -v— 18/18 pass
(The repo-wide dashboard //go:embed dist build-order artifact is pre-existing and unrelated to this PR.)
Prior findings — all addressed
- Medium —
go mod tidy:pkg/browseris now a committed direct dependency (ebacfdfe). ✅ - Medium — docker container-only ports:
publishedHostPortnow defers to the canonicalsvc.GetPortMappings(), skipsHostPort <= 0(so a docker bare port like"80"yields no URL instead ofhttp://localhost:80), and filters non-TCP protocols. ✅ - Low — swallowed
azure.yamlparse error: newparseOpenAzureYamlusesdetector.FindAzureYaml, returns(nil, nil)for a missing file, and surfaces real parse errors (TestResolveOpenServiceURLSurfacesParseError). ✅ - Low —
RawPathstaleness injoinOpenURLPath:parsed.RawPathis now cleared after updatingPath, with a test asserting re-encoding. ✅ - Nit — output channel:
--printnow writes viafmt.Fprintln(cmd.OutOrStdout(), …)with write-error handling. ✅ - Nit — ignored
context: the unusedcontext.Contextparameter was removed. ✅ - Test gaps: excellent table-driven coverage added —
TestOpenURLFromService(11 cases incl. docker bare-port, UDP, IPv6/bind-IP, protocol suffix, malformed spec),TestJoinOpenURLPath(query/fragment/re-encode/error cases),--printand browser-launch seams, not-found service listing, and parse-error surfacing. ✅
Optional (non-blocking) nits
publishedHostPortdiscards theboolfromGetPortMappings()— intentional and correct, since iterating an empty slice is safe.azure.yamlis parsed twice (once insideGetServiceInfo, once inparseOpenAzureYaml) — minor redundancy kept for clearer error surfacing; fine as-is.- Running-state URLs from
bestOpenURLaren't scheme-validated the way theazure.yamlpath is (validated upstream byParseAzureYaml); negligible in a local-dev/localhost context.
Clean, well-tested feature that fully incorporates the review feedback. Nice work.
Closes #378 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 340b4dd3-b4b3-4f1a-9163-66b16d96fa81
pkg/browser is now imported directly by the open command, so it belongs in the direct require block. Preflight's tidy gate was failing on this. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1bbad110-6dc8-4b15-abdd-576ee731eeaa
…rrors Use service.GetPortMappings so port specs go through the canonical parser. A Docker container-only port such as "80" has an auto-assigned host port, so it no longer resolves to http://localhost:80. Bind IPs, IPv6 binds, protocol suffixes and malformed specs are now handled consistently, and non-TCP mappings are skipped. Parse azure.yaml explicitly after the running-state lookup so a malformed file reports the real error instead of "service not found", while a project with no azure.yaml still reports "not found". Clear url.URL.RawPath after joining --path so String() re-encodes from the updated Path. Route --print through cmd.OutOrStdout and check the write error, and drop the unused context.Context parameter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1bbad110-6dc8-4b15-abdd-576ee731eeaa
Convert to table-driven testify tests per AGENTS.md and close the gaps called out in review: --print output, browser launch, the not-found "Available services" list, malformed azure.yaml, port mapping edge cases, and path-with-query/fragment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1bbad110-6dc8-4b15-abdd-576ee731eeaa
ac7154e to
dac97ac
Compare
wbreza
left a comment
There was a problem hiding this comment.
Code Review (re-review after rebase) — feat(open): add service URL opener
Review outcome: Approve. The only change since my previous approval is a rebase onto the latest main (pulling in ~20 merged feature PRs and dependency upgrades). The PR's own contribution is unchanged, and I re-verified it still builds and passes tests against the new base.
What changed since last review
- Branch rebased onto latest
main(new headdac97ac). The PR's true delta vs the current merge-base is the same four files I already approved:open.go(+215),open_test.go(+323),main.go(+1, registration),go.mod(+1/-1, directpkg/browser). open.gois byte-for-byte identical to the version approved atac7154e— no source changes to the feature itself.
Re-verification against new main (isolated worktree at dac97ac)
go build ./src/cmd/app/commands/— passgo build ./src/internal/service/... ./src/internal/serviceinfo/... ./src/internal/detector/...— passgo vet ./src/cmd/app/commands/— passgo test ./src/cmd/app/commands/ -run Open -v— all Open tests pass (9 functions / 20 subtests)
The dependency upgrades and service/serviceinfo/detector changes on main introduced no API drift: GetPortMappings() ([]PortMapping, bool), PortMapping.HostPort/Protocol, FindAzureYaml, LocalServiceInfo.CustomURL/URL/Port, and LocalServiceConfig.CustomURL are all intact and compatible.
(The repo-wide dashboard //go:embed dist build-order artifact is pre-existing and unrelated to this PR.)
All findings from earlier rounds remain resolved. Clean rebase — good to merge.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #552 +/- ##
==========================================
+ Coverage 61.84% 61.91% +0.07%
==========================================
Files 224 225 +1
Lines 29430 29545 +115
==========================================
+ Hits 18202 18294 +92
- Misses 9980 9991 +11
- Partials 1248 1260 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
🚀 Test This PRA preview build ( 🌐 Website PreviewLive Preview: https://jongio.github.io/azd-app/pr/552/ One-Line Install (Recommended)PowerShell (Windows): iex "& { $(irm https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/install-pr.ps1) } -PrNumber 552 -Version 0.20.0-pr552"Bash (macOS/Linux): curl -fsSL https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/install-pr.sh | bash -s 552 0.20.0-pr552UninstallWhen you're done testing: PowerShell (Windows): iex "& { $(irm https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/uninstall-pr.ps1) } -PrNumber 552"Bash (macOS/Linux): curl -fsSL https://raw.githubusercontent.com/jongio/azd-app/main/cli/scripts/uninstall-pr.sh | bash -s 552Build Info:
What to Test: |
The 15 commits that landed on main while this branch was open added CLI surface the docs gate correctly rejected. Resolve the cli-reference.md conflict by keeping both sides, then backfill everything the gate found. Commands documented: - open (#552), including cli/docs/commands/open.md and the URL resolution order - website reference pages for hooks (#515) and remove (#511), which shipped without generated pages Flags documented: - run --env (#498), run --no-deps (#544) - logs --summary (#491), logs --min-level (#527), logs --no-timestamps (#561) - status --exit-code (#563) - env --prefix (#490) Also fix the gofumpt failure in env_test.go that currently breaks mage preflight on main. mage preflight passes clean. Docs gate covers 31 commands, website validation covers 27. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds azd app open for opening a resolved local service URL. The command supports --path for routes and --print for scripts, and reports a clear error when a service has no URL source.
Closes #378
Validation: go test ./src/cmd/app/commands -run 'Test(ResolveOpenServiceURLFromCustomURL|ResolveOpenServiceURLFromPorts|ResolveOpenServiceURLMissingURL|JoinOpenURLPath)'