Repository navigation
feat(cli): create environments with environments create - #123
thebrandonlucas wants to merge 1 commit into
Conversation
tee8z
left a comment
There was a problem hiding this comment.
This is close. The request body matches the contract's Environment schema, a unit test pins it, and the 400, 409, 401, and 403 paths, the closed connection, and both kinds of API key each have an integration test. The changelog and guides are updated, and the description reports the gate and the CRAP result.
CI: The aarch64-apple-darwin failure is a broken runner, not this PR. xcrun --sdk macosx --show-sdk-path could not find the SDK, so the build script failed to link before any test ran. Rerun the job.
Inline:
- Should fix: an answer that cannot be read after the request arrived loses the "may have been created" detail.
- Nit: Ctrl-C records the profile's environment IDs.
- Nit: the description hides an
--accounterror.
Needs a decision before merge (your open question): docs/quality.md says to record a mutation that carries an ID before sending it, and to reject a reused ID with a different payload. This PR does neither, so --id X sent twice with different names is not caught locally. Either add the journal entry, or record the exception in docs/quality.md with the reason: the auth service rejects a duplicate ID with a 409.
|
On the open journal question from your review: this still needs a decision. Here is what I found to help make it. What the auth service does with the ID, from
The options:
My recommendation is option 2, plus an auth-service issue for the 409. I have left this unresolved for you and Brandon to decide. |
|
Decision on the journal question: option 2. The exception is recorded under Approved exceptions in |
tee8z
left a comment
There was a problem hiding this comment.
looking good, just some small stuff
4859ddc to
db2ff91
Compare
tee8z
left a comment
There was a problem hiding this comment.
Reviewed current head db2ff91 against #130 (c94b96e), including the newly pushed auth/execution refactor.
Requesting changes for the inline recovery bug: a valid JSON 5xx response is reported as a definite failure and loses the environment ID, although a gateway can return that after upstream creation. A loopback-only reproduction recorded two different environment IDs on two manual runs while both CLI results said “did not create,” exit 1, with detail: null. The suggested fix preserves uncertainty and the recovery ID; please add regression coverage.
Repository requirement: git diff --shortstat c94b96e...db2ff91 reports 1,017 additions + 73 deletions = 1,090 changed lines. Root AGENTS.md requires the full PR diff to remain under 1,000; split out a preparatory layer or reduce the diff before merge.
Local verification passed on db2ff91: formatting, Clippy with warnings denied, dependency hygiene, cargo test --locked (222 unit + 166 integration), and contract/docs checks. Required CRAP measurement also passed: 928/1162 functions measured, 234 unmeasured, zero scores >=20; highest 18.10 (wait_for_payment). Passing tests currently miss the JSON-5xx recovery case.
Limits: no live staging creation; the recovery reproduction uses a synthetic loopback service. No manual Windows/macOS run.
| let code = response | ||
| .error_code() | ||
| .unwrap_or_else(|| "request_rejected".into()); |
There was a problem hiding this comment.
[P2] Preserve uncertainty for JSON 5xx creation responses
A gateway can return a valid JSON 502/504 after the upstream has committed the create. This path currently treats that as a definitive rejection, says “did not create,” and drops the generated ID (detail: null). The HTML-502 path immediately above correctly preserves uncertainty, so changing the gateway's response format changes the recovery guarantee.
Reproduced at db2ff91 using only a loopback mock: the mock recorded each submitted environment before returning 502 {"error":"bad_gateway"}. The CLI returned exit 1 with no recovery ID; repeating the same command without --id submitted a second, different ID. No live service was contacted.
Treat server/gateway 5xx answers as uncertain, preserve the ID and list-before-rerun hint, and return transport/unknown exit 4. For example, insert the following before the definitive rejection handling, then add integration cases for JSON and empty 5xx bodies that assert the ID, unknown outcome, and no automatic resend:
| let code = response | |
| .error_code() | |
| .unwrap_or_else(|| "request_rejected".into()); | |
| let code = response | |
| .error_code() | |
| .unwrap_or_else(|| "request_rejected".into()); | |
| if response.status >= 500 { | |
| return Err(environment.possibly_created(Error::transport(format!( | |
| "Environment creation failed (HTTP {}, {code})", | |
| response.status | |
| )))); | |
| } |
Add `voltage environments create --org ORG --name NAME [--id UUID]`, which posts the auth contract's Environment to the auth service with the login's organization token. Without --execute or VOLTAGE_EXECUTE=1 it describes the request and exits 6. The CLI generates the environment ID and reports it in resource_id.
db2ff91 to
d6a4f61
Compare
tee8z
left a comment
There was a problem hiding this comment.
Reviewed d6a4f611bb749183eec2f410004ff9f54bbfb533 against master 2f7ca726f3cd2807fe13b9e5f68c0b076c5cf7b0. The JSON-5xx recovery finding remains on this head; see the inline suggestion and loopback reproduction. A JSON gateway response must retain the same uncertainty and generated recovery ID as an unreadable response. Manual reruns otherwise create fresh IDs while the CLI says nothing was created.
The full PR remains above the repository limit: 1,017 additions + 73 deletions = 1,090 changed lines. Root AGENTS.md requires fewer than 1,000 against the actual PR base. Split a preparatory change or reduce this diff before merge.
Independent verification on this head: formatting, full-target Clippy with warnings denied, dependency hygiene, contract/docs check, and full cargo test --locked passed (222 unit + 166 integration). These passing tests omit JSON 5xx creation responses. Required whole-package CRAP measurement passed: 928 measured functions, 234 unmeasured, zero scores >=20; highest 18.099272 (wait_for_payment). Scope: src, build.rs, tests; cargo-llvm-cov 0.8.5 and knots 1.16.0, all targets/features, locked/offline.
No live staging creation, production request, or cross-platform manual run. The reproduction used only synthetic credentials and a loopback mock.
| } | ||
| let code = response | ||
| .error_code() | ||
| .unwrap_or_else(|| "request_rejected".into()); |
There was a problem hiding this comment.
[P2] Preserve uncertainty and the environment ID for JSON 5xx responses
The current head still reaches the definitive-rejection branch for a valid JSON 502/504. A gateway can return that after the upstream committed creation. I reproduced this on the unmodified head with a loopback service that records each POST then returns 502 {"error":"bad_gateway"}: two manual runs exited 1 with detail:null and “did not create,” while the mock recorded two distinct environment IDs. No automatic retry occurred, but the lost recovery ID makes a manual rerun unsafe.
Insert a server-error branch before the definitive rejection, and cover JSON and empty 5xx bodies with regression tests asserting unknown outcome, the original ID, a list-before-rerun hint, and exactly one POST:
| .unwrap_or_else(|| "request_rejected".into()); | |
| .unwrap_or_else(|| "request_rejected".into()); | |
| if response.status >= 500 { | |
| return Err(environment.possibly_created(Error::transport(format!( | |
| "Environment creation failed (HTTP {}, {code})", | |
| response.status | |
| )))); | |
| } |
Problem
voltage environmentshas onlylist. To add an environment, a person had to leave the CLI, even though the auth contract (api/auth-openapi.json) definesPOST /api/v1/organizations/{organization_id}/environments.Closes #118. Linear: DEV-2953.
Behavior
--execute(or withVOLTAGE_EXECUTE=0)outcome: dry_run. The name is[OMITTED]unless--show-secrets.--executeorVOLTAGE_EXECUTE=1{environment_id, organization_id, name}once.VOLTAGE_API_KEY)environments create needs a browser login, ..., the same wording asenvironments list. Sends nothing.The auth service did not create the environment (HTTP STATUS, CODE). A 403 also gets a permission hint.uncertain_submissiondetail. Without a hint of its own, it says to look for the ID withenvironments listbefore a rerun.--org, no--name, blank--name, or a bad--id--account typo, or several saved logins and no--account)environment_idrequired, so the CLI generates it, or uses--id UUID. Both the description and the result report it inresource_id. The result'sdatais the environment that was sent; the 201 response has no documented body. On a terminal, the result suggestsvoltage profiles create PROFILE --org ORG --env ID.--output-file, as API changes already do. Ctrl-C after sending reports the possible write with the environment ID and no environment IDs, because the change is to the auth service, not to the profile's environments.Code changes
src/environment.rs(new): describe or sendenvironments create.auth.rs:AuthService::create_environmentandResolver::create_environment. The API-key error moved out ofdiscoverintorequires_login, soenvironments listandcreateshare it.send_to_authlets a write report its own transport failure. The contract test also checks that the request body has exactly theEnvironmentschema's required fields.api.rs:Execution::change,not_sent, andsending_from_environmentare shared with the API path.SubmittedWritenow storespayment: boolinstead of anOperationId, so an auth-service write can be recorded too.dry_run.rs:announce_changeis the terminal "Not sent: this would ..." line, without anApiInvocation.login-and-profiles.md, troubleshooting,docs/commands.md(regenerated), and aCHANGELOG.mdline.Try it on staging
Setup: Try a pull request on staging
Find an organization ID with
voltage organizations list --account me. These runs only describe; nothing is sent:Expect:
! Not sent: this would create an environmentwith the generatedenvironment_id, thenexit 6."outcome": "dry_run", aresource_idequal tobody.environment_id, and"name": "[OMITTED]".Unknown credential typowith the saved credential names, thenexit 3.the name must not be empty, thenexit 2.--executeexample.Sends a change. This creates a real environment in ORG_ID, and the CLI cannot delete it. Run it only in an organization where a test environment is acceptable:
Expect: the result shows
environment_idand aSave it in a profilenext step. The new ID appears in the list.Validation
Run on the head after merging
masterand the review fixes (fc9e3c5):cargo fmt --check,cargo clippy --locked --all-targets -- -D warnings,cargo machete --with-metadata,cargo test --locked(200 unit, 156 integration), andpython3 scripts/check-coverage.py: all pass.--executeand withVOLTAGE_EXECUTE=1; generated ID; table output; rejection with 400, 409, 401, and 403; a connection that closes before a response; a 502 HTML page and an oversized body after the request (environments_create_reports_an_unusable_answer_with_its_id); a description with an unknown or ambiguous login; API key (saved and ambient); and missing or invalid flags and scope. A unit test checks the Ctrl-C detail for an auth change.scripts/crap-report.sh, cargo-llvm-cov 0.8.5, knots 1.16.0): 863 of 1084 functions measured, 0 violations. The highest is 18.10,wait_for_paymentinsrc/api.rs, which this PR does not change. New or changed functions:environment::create9.01,describe5.0,AuthService::create_environment4.0,auth_url3.0,NewEnvironment::possibly_created2.0.--executerun was not done, because the CLI cannot delete an environment.Limits and open questions
OperationIdand reconcile through the Voltage API, and an environment is neither. Recovery after a lost response isenvironments listplus the reported ID. The auth service's unique constraint already prevents a duplicate environment. The exception is recorded under Approved exceptions indocs/quality.md. A reused--idcurrently gets HTTP 500 rather than 409; that is tracked in voltagecloud/auth-service#428.environment_idas the environment'suuid, so the reportedresource_idis the real ID. The column isUNIQUE, so a reused--iddoes not create a second environment. The server returns HTTP 500 for it, not 409 (voltagecloud/auth-service#428).SubmissionStatepath as API writes.EnvironmentCommandflattens feat(cli): pick the wallet, organization, or environment to show on a terminal #126'sDiscoveryCommand, soenvironments listkeeps fix(cli): accept --all on environments, organizations, and profiles list #120's--all(all_at_once_arg()) andenvironments getkeeps the picker; fix(cli): accept --all on environments, organizations, and profiles list #120's parse test now matchesEnvironmentCommand::Discovery(DiscoveryCommand::List). With feat(cli): print a profile's scope as dotenv lines with context --format env #129 and feat(cli): link a prefilled GitHub issue from voltage feedback and panics #130 below it,startup::executereached CRAP 20, soSession::create_environmentnow runs this command (50fbb34). The highest CRAP score on this head is 18.10 (wait_for_payment); no function is at or above 20.PUT(update or rename) is deliberately left out.--datais not offered: the body has three fields, all covered by flags and scope.Stack
Stack #135, layer 8 of 9 (bottom targets
master):--allonorganizations,environments, andprofiles listVOLTAGE_*variablevoltage context --format envvoltage feedbackand panicsvoltage environments createflake.nixwith a KaifileThe layers are chained with merge commits, because history is not rewritten. Use squash and merge, bottom first.