Fix two behaviors PR 2's tests pinned as quirks#28
Merged
Conversation
esp's purpose is to expose SSM params as environment variables,
and env var names cannot contain hyphens. formatParamName already
uppercased un-prefixed names; extend the same step to replace
hyphens with underscores so callers like
esp put -n my-app-key -v ...
produce a valid env var name (ACME_MY_APP_KEY) rather than the
half-normalized ACME_MY-APP-KEY that survived before.
The HasPrefix branch is unchanged: callers who pass a fully-formed
name keep it verbatim, hyphens and all. TestEspName updated to
pin the new rule and adds a multiple-hyphens case plus an
already-prefixed-with-hyphens case that documents the asymmetry.
Surfaced by writing the PR 2 tests; design doc described this
conversion but the code wasn't doing it.
checkSSMByPathError was the only mapper without a trailing smithy.APIError catch — its sibling functions (checkBaseSSMErrors, checkSSMGetParameterError, checkSSMPutParameterError, checkDeleteParameterError) all fall through to a generic API error before returning nil. The asymmetry meant any GetParametersByPath failure not in the explicit list (throttling, auth, region issues) would map to nil and be silently swallowed by callers using this mapper. Today the dispatcher (checkSSMError) does not actually route GetMany to this function, so the fix is mostly defensive. Worth doing now: the function is exported-shaped and any future caller would inherit the swallow bug otherwise. TestCheckSSMByPathError updated; the case that previously asserted "returns nil (no fallback in this function)" now asserts fallthrough-and-return, matching the siblings. Surfaced by writing the PR 2 tests.
AbsolutOD
force-pushed
the
fix/hyphens-and-bypath-fallback
branch
from
e37b0ba to
08a83a2
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
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
Stacked on #27. Fixes the two behaviors PR 2 flagged as "notable items pinned by the new tests":
formatParamNamenow converts hyphens to underscores. esp's purpose is to expose SSM params as environment variables; env var names cannot contain hyphens. The function already uppercased un-prefixed names — extending the same step to replace hyphens makes the output a valid identifier.esp put -n my-app-key -v ...now producesACME_MY_APP_KEYinstead of the half-normalizedACME_MY-APP-KEY. TheHasPrefixbranch is unchanged: callers passing a fully-formed name keep it verbatim.checkSSMByPathErrornow has thesmithy.APIErrorfallback its siblings already have. Previously this was the only mapper missing the trailing generic-API catch, so anyGetParametersByPathfailure outside the explicit list (throttling, auth, region issues) mapped toniland would be silently swallowed by callers using this mapper. The dispatcher does not currently routeGetManyhere, so today this is defensive — but the function is shaped to be reused, and any future caller would have inherited the swallow bug.Both behaviors were the design doc's stated intent; the code shipped without them. Tests added in #27 are updated in lockstep — the assertion that previously read "hyphens preserved" now reads "hyphens become underscores", and the assertion that read "no fallback in this function" now reads "falls through and is returned".
Stacking
PR base is
feature/tests-pure-logic(#27). After #27 merges, GitHub will automatically retarget the base of this PR tomain.Test Plan
go build ./...cleango vet ./...cleango test ./...— all packages pass; the two updated table cases now assert the new behaviorCommits
fix(cmd/put): normalize hyphens to underscores in formatParamNamefix(internal/ssm): add smithy.APIError fallback to checkSSMByPathError