fix(cli)!: address CLI and config robustness gaps (breaking: JSON-mode buy/create now requires --yes)#537
Open
Phil-Browne wants to merge 17 commits into
Open
fix(cli)!: address CLI and config robustness gaps (breaking: JSON-mode buy/create now requires --yes)#537Phil-Browne wants to merge 17 commits into
Phil-Browne wants to merge 17 commits into
Conversation
Save() now writes to a temp file in the same directory and renames it into place, so a crash mid-write cannot corrupt the credentials file. It also chmods the file to 0600 on every save, so a pre-existing over-permissive file gets re-tightened rather than left as-is.
Previously --env login could take the access key from the environment while falling back to the active profile for the secret key (or vice versa) when only one var was set. Both halves must now come from the same source, or login errors instead of silently mixing credentials.
locations get iterated the raw []*megaport.LocationV3 without the nil check that filterLocations already applies, so a nil entry in the response could panic. Skip nil entries the same way filterLocations does.
Exit-code classification previously matched "unknown flag"/"invalid"/ "arg(s)" against error text, so an API error that happened to contain those words could be misclassified as a usage error. Flag-parse failures and missing-required-flag errors are now tagged as a typed usage error at the point cobra generates them (SetFlagErrorFunc and a proactive ValidateRequiredFlags check), and ParseInt now returns a typed *ValidationError instead of a plain error, so classifyError can key off error types instead of wording. isCobraUsageError's substring match remains only as a fallback for the "unknown command" and "arg(s)" cases that run before those hooks and have no typed equivalent.
JSON and json-file input skipped the purchase confirmation prompt entirely, so a JSON-mode buy without --yes went straight through with no interactive prompt and no explicit acknowledgement. Every buy/create command (port, LAG port, VXC, MVE, IX, MCR, NAT Gateway) now requires --yes to proceed in JSON mode, and returns a usage error otherwise. The check runs as early as each function's data flow allows, ahead of login and live validation calls, so a doomed request fails fast instead of spending a network round trip first.
FindDocFile decided whether to clean up a doc file by testing its path for a "megaport-docs-" substring, which is fragile if a real doc file or working directory happened to contain that string. It now returns an explicit isTemp bool and cleanup is driven by that instead.
printXML wrote struct json tag names directly as XML element local names. Go's encoding/xml does not validate element names at encode time, so a tag starting with a digit, containing punctuation, or colliding with another tag after sanitization would silently produce malformed or ambiguous XML. Sanitize each name against the XML Name grammar (with collision disambiguation across a struct's fields) once per printXML call, for both the native and WASM renderers.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #537 +/- ##
==========================================
+ Coverage 78.57% 78.90% +0.33%
==========================================
Files 192 193 +1
Lines 18522 18646 +124
==========================================
+ Hits 14553 14712 +159
+ Misses 2904 2863 -41
- Partials 1065 1071 +6
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens several CLI robustness edges across config persistence, credential selection, exit-code classification, confirmation flow for JSON-driven purchases/creates, and XML output generation.
Changes:
- Make config saves atomic (temp file + rename) and re-tighten permissions to
0600. - Require
--yeswhen using--json/--json-filefor buy/create flows to prevent accidental purchases/creates without confirmation. - Improve robustness via typed/structured errors (validation + cobra flag parsing) and sanitize XML element names derived from struct tags.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/validation/parse.go | Switch ParseInt failures to typed *ValidationError. |
| internal/validation/parse_test.go | Update ParseInt tests to assert typed validation errors and updated messaging. |
| internal/commands/vxc/vxc_actions.go | Require --yes for JSON-mode VXC purchases; keep interactive confirmation for non---yes. |
| internal/commands/vxc/vxc_actions_test.go | Update/add tests covering JSON + --yes and JSON without --yes usage error. |
| internal/commands/users/users_actions_test.go | Update expected validation error casing (“Invalid …”). |
| internal/commands/ports/ports_prompts_test.go | Update expected validation error casing (“Invalid …”). |
| internal/commands/ports/ports_actions.go | Require --yes for JSON-mode port/LAG purchases; keep confirmation for non---yes. |
| internal/commands/ports/ports_actions_test.go | Update/add tests for JSON + --yes and JSON without --yes usage error. |
| internal/commands/partners/partners_actions_test.go | Update expected validation error casing (“Invalid …”). |
| internal/commands/nat_gateway/nat_gateway_test.go | Require --yes for JSON-mode NAT Gateway create; add negative test. |
| internal/commands/nat_gateway/nat_gateway_additional_test.go | Ensure JSON-mode NAT Gateway create tests set --yes. |
| internal/commands/nat_gateway/nat_gateway_actions.go | Enforce --yes requirement in JSON/JSON-file mode; simplify confirmation gating. |
| internal/commands/mve/mve_prompts_test.go | Update expected validation error casing (“Invalid …”). |
| internal/commands/mve/mve_actions.go | Require --yes for JSON-mode MVE purchase; keep confirmation for non---yes. |
| internal/commands/mve/mve_actions_test.go | Update/add tests for JSON + --yes and JSON without --yes usage error. |
| internal/commands/mcr/mcr_prompts_test.go | Update expected validation error casing (“Invalid …”). |
| internal/commands/mcr/mcr_actions.go | Require --yes for JSON-mode MCR purchases; keep confirmation for non---yes. |
| internal/commands/mcr/mcr_actions_test.go | Update/add tests for JSON + --yes and JSON without --yes usage error; update casing. |
| internal/commands/mcr/mcr_actions_prefix_filter_test.go | Update expected validation error casing (“Invalid …”) for stderr assertions. |
| internal/commands/locations/locations_actions.go | Skip nil location entries to avoid panic when scanning response. |
| internal/commands/locations/locations_actions_test.go | Add coverage ensuring nil entries don’t panic; update casing. |
| internal/commands/ix/ix_test.go | Update expected validation error casing (“Invalid …”). |
| internal/commands/ix/ix_actions.go | Require --yes for JSON-mode IX purchases; keep confirmation for non---yes. |
| internal/commands/ix/ix_actions_test.go | Update/add tests for JSON + --yes and JSON without --yes usage error. |
| internal/commands/config/manager.go | Implement atomic config save (temp + rename) and re-chmod to 0600. |
| internal/commands/config/manager_test.go | Adjust read-only test to directory permissions; add tests for chmod retightening and temp cleanup. |
| internal/commands/config/login.go | Prevent mixing env/profile credential halves when --env is used; error on partial env vars. |
| internal/commands/config/login_test.go | Add tests ensuring partial env vars error and don’t mix with profile. |
| internal/base/output/output.go | Sanitize XML element names before encoding (non-WASM). |
| internal/base/output/output_wasm.go | Sanitize XML element names before encoding (WASM). |
| internal/base/output/output_test.go | Add tests for illegal/colliding XML element names and sanitizer behavior. |
| internal/base/output/common.go | Add XML name sanitization + collision disambiguation helpers. |
| internal/base/cmdbuilder/docs_render.go | Replace temp-file cleanup substring heuristic with explicit isTemp flag. |
| cmd/megaport/megaport.go | Tag cobra flag-parse/required-flag errors as typed usage errors (early), reducing substring-based classification. |
| cmd/megaport/megaport_test.go | Add tests asserting unknown flags / missing required flags become typed usage errors. |
- quote the offending value in ParseInt's validation error so an empty or whitespace-only input isn't ambiguous - wrap the --env partial-credential guard in a typed usage error instead of a plain error - assert io.EOF specifically when checking XML output is well-formed, so a malformed document with an early syntax error can't slip past the same as a valid one - add exit-code assertions to the partial-env-var login tests to cover the new typed usage error
os.File.Write can return n < len(data) with a nil error; check the byte count so a short write doesn't get silently renamed into place as the new config file.
Pre-formatting the value with %q before handing it to NewValidationError corrupted the typed field: any caller inspecting ValidationError.Value got a quoted/escaped display string instead of the actual offending input. Pass the raw value through, matching every other validator in the package.
The "neither env var set" subtest reached the real Authorize() call and relied on whatever network error came back, making it environment-dependent and possibly slow or flaky under CI egress restrictions. Point utils.BaseURL/utils.TokenURL at a local httptest server returning 401 so it fails fast and deterministically while still proving the code path reaches Authorize.
…rror Keeps ValidationError.Value raw (the caller passes the original string unmodified) but quotes it in Error() only when it is empty or whitespace-only, so the message doesn't read as a bare, ambiguous dash.
The comment claimed every RunE in the codebase is wrapped by utils.Wrap*. The cmdbuilder-injected docs subcommand and the --generate-skeleton bypass are unwrapped exceptions; note them and why the substring match is still safe.
BuyPort, BuyLAGPort, BuyIX, BuyMCR, BuyMVE, and BuyVXC each repeated the same --yes check for JSON-mode purchases. Extract it into utils.RequireYesForJSONBuy.
TestReadOnlyConfigFile and TestSaveRetightensOverPermissiveFile rely on chmod semantics and permission-denied error text that don't hold on Windows, so skip them there rather than let them fail or be meaningless for local Windows contributors.
… state This test runs the real rootCmd through PersistentPreRunE, which reads the config dir and mutates shared output config globals. Point it at a temp config dir and reset output state afterward, matching the sibling PersistentPreRunE tests in this file.
Collaborator
|
Code is sound; the behaviour change needs a release note. |
Flag help text and generated docs for every buy/create command didn't mention that --yes is required in JSON mode, even after the enforcement went in. Update the shared --yes flag description and NAT Gateway's create flag, then regenerate docs/.
Phil-Browne
changed the title
Contributor
Author
|
Agreed this needs to be surfaced, but
Version bumps aren't done per-PR here, they happen via a git tag cut in a separate release-prep step, so there's nothing to bump in this branch. |
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
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.
Hardens a batch of small robustness gaps in the config, login, and output plumbing found during a hygiene pass.
--envlogin now requires both the access key and secret key to come from the same source, instead of possibly mixing one from the environment and one from the active profile.locations getskips nil entries in the response instead of risking a panic.*ValidationErrorfromParseInt) instead of matching substrings like "invalid" or "arg(s)" against error text, so an API error that happens to contain those words won't be misclassified as a usage error.--yesto proceed when using--json/--json-file, closing a gap where JSON-mode input skipped purchase confirmation entirely.isTempflag instead of a path substring check.printXMLnow sanitizes struct tag names against the XML Name grammar (with collision disambiguation) before using them as element names, since Go'sencoding/xmldoesn't validate this itself.Test plan
go build ./...go test ./...golangci-lint runGOOS=js GOARCH=wasm go build -tags js,wasm ./...