lakebox: bugfixes — start help text, create default-clobber, keyHash trailing-newline, plus UX#5460
Open
akshaysingla-db wants to merge 1 commit into
Conversation
…sh trailing-newline, plus UX
Three correctness fixes (BLOCKERs from a final ship-readiness audit) and
several smaller UX cleanups, bundled because each is small and they all
sit on the same `register → ssh` path that gets exercised together.
BLOCKERs:
1. `start.go` `Long:` said "blocks until the sandbox reaches Running
(or up to 5 minutes)" while the actual `startWaitTimeout` constant
is 10 minutes. Documented bound mismatched the enforced bound — fix
the text.
2. `create.go` was using `if _, err := api.get(currentDefault); err != nil`
to decide whether to overwrite the saved default. Any non-404 error
(transient 5xx, network blip, rate limit) silently clobbered the
user's default. Every sibling (`delete`, `default`, `ssh`) already
uses `errors.Is(err, apierr.ErrNotFound)` for this — `create` now
matches.
3. `keyHash` included a trailing newline in its sha256 input for `.pub`
files without an OpenSSH comment. Locally-generated keys are fine
(`ssh-keygen -C "lakebox"` always adds a comment), but a user-supplied
key without `-C` would produce a hash that doesn't match the server's,
making `verifyKeyRegistered` falsely report "key not registered". Fix:
`strings.TrimSpace` the input first. New test cases pin the behavior
for `\n` and surrounding whitespace.
UX fixes:
- `status.go` displayed an `fqdn` field even though `api.go` documents
FQDN as the manager's internal routing host — drop from non-JSON
output (still present in `-o json` for completeness).
- `status.go` wrapped 404 errors as "failed to get lakebox X: ..." while
`delete`, `default`, and `ssh` all return the friendlier "no lakebox
named X — `databricks lakebox list` shows available IDs". Add the
same branch.
- `ui.go` `warn()` used cyan, the same color as `ok()` and the spinner
✓ marker — so a warning was visually indistinguishable from success.
Switch to yellow (already in the palette via `cmdio.Yellow`).
- `delete.go` returned `errors.New("aborted")` on user-cancel, which
surfaced as `Error: aborted` with a non-zero exit. Print "Cancelled."
and return nil instead.
- Inconsistent capitalization across `warn()` messages (some sentence
case, some lowercase) — standardize on sentence case to match the
rest of the repo.
Co-authored-by: Isaac
Contributor
Waiting for approvalBased on git history, these people are best suited to review:
Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
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.
Summary
Three correctness fixes and several small UX cleanups, surfaced by a final ship-readiness audit. Bundled because each is small and they all sit on the same
register → sshpath that gets exercised together.BLOCKERs (real bugs)
1.
start.godocumented timeout was half the actual timeoutLong:said "blocks until the sandbox reaches Running (or up to 5 minutes)" whilestartWaitTimeout = 10 * time.Minute. Fix the text.2.
create.goclobbered the saved default on any transient API errorPrevious code:
A 500, network blip, or rate-limit silently overwrote the user's chosen default. Every sibling (
delete,default,ssh) already useserrors.Is(err, apierr.ErrNotFound)—createnow matches.3.
keyHashincluded a trailing newline in its sha256 input.pubfiles end with\n. Locally-generated keys are fine becausessh-keygen -C "lakebox"always adds a comment that gets stripped before the\nis reached. But a user-supplied key without-Cwould produce a hash that doesn't match the server's, makingverifyKeyRegisteredfalsely report "key not registered". Fix:strings.TrimSpacethe input first. Two new test cases pin the behavior for\nand surrounding whitespace.UX fixes
status.goexposedfqdn—api.godocuments FQDN as the manager's internal routing host, not user-actionable. Dropped from non-JSON output (still in-o jsonfor completeness).status.go404 message — wrapped as "failed to get lakebox X: ..." whiledelete/default/sshreturned the friendlier "no lakebox named X —databricks lakebox listshows available IDs". Now consistent.ui.go warn()switched to yellow — was cyan, indistinguishable from the success ✓ marker.delete.goclean cancel — returningerrors.New("aborted")surfaced asError: aborted+ non-zero exit. Now printsCancelled.and returns nil.warn()messages.Test plan
go test ./cmd/lakebox/...passes (including 2 new keyHash cases for trailing-newline and leading/trailing whitespace)go build ./...cleangofmt -l cmd/lakebox/cleanOut of scope
Acceptance test scaffold for
cmd/lakebox/(the bigger gap surfaced by the same audit) is its own PR — these are zero today and adding them is a separate larger change.This pull request and its description were written by Isaac.