[VPEX][14] Accept bare serverless version numbers (5, not v5)#5965
Conversation
Integration test reportCommit: 840642f
10 interesting tests: 4 RECOVERED, 4 SKIP, 2 flaky
Top 1 slowest tests (at least 2 minutes):
|
47c9f11 to
3f6b218
Compare
2442192 to
1189676
Compare
3f6b218 to
6ff8a75
Compare
1189676 to
67c2589
Compare
anton-107
left a comment
There was a problem hiding this comment.
Reviewed as part of the full stack (#5960–#5965). Requesting changes: this PR breaks CI — two existing unit tests fail at this commit, contradicting the "unit + acceptance green" claim in the description.
-
[blocker]
TestResolveBundleServerlessandTestResolveJobServerlessEmptyVersionFallsBackToDefaultfail. Runninggo test ./libs/localenv/at this commit:--- FAIL: TestResolveBundleServerless (target_test.go:100) expected: "serverless/serverless-5" actual: "serverless/serverless-v5" --- FAIL: TestResolveJobServerlessEmptyVersionFallsBackToDefault (target_test.go:153) expected: "serverless/serverless-5" actual: "serverless/serverless-v5" FAIL github.com/databricks/cli/libs/localenvRoot cause: this PR changes
defaultServerlessVersionfrom"v5"to"5"(libs/localenv/envkey.go:23). Those two assertions (which this PR does not touch) build the expected key as"serverless/serverless-"+defaultServerlessVersion→ nowserverless/serverless-5, whileResolveTargetstill runs the value throughNormalizeServerless(which re-adds thev) →serverless/serverless-v5. Both tests pass at the parent commit (#5964), so this PR is what breaks them. Fix: assert the literal"serverless/serverless-v5", or wrap the constant inEnvKeyForServerless(...)on the expected side. -
[low] Those tests are self-referential. As written they compare the code against the same constant the code uses, so they'd pass for any value of
defaultServerlessVersion— they don't actually pin the default to v5. Worth asserting the concrete expected env key (serverless/serverless-v5) so the default is genuinely covered. (Related: there's no acceptance test that servesserverless-v5; see my note on #5962.) -
[low]
NormalizeServerlessstill does no validation. Now that the bare form is the documented input, malformed values pass straight through:--serverless-version vv5→serverless/serverless-vv5,v→serverless/serverless-v," 5"→serverless/serverless-v 5(whitespace not trimmed).ResolveTarget's serverless branch (target.go:106) accepts any non-empty string, so these resolve "successfully" at the resolve phase and only fail two phases later at fetch with a genericE_ENV_UNSUPPORTED404 that never tells the user their version string was malformed. Per the repo's "reject incompatible inputs early with an actionable error" rule, validating the format at resolve/preflight would be better.
Reviewed with AI assistance (build + unit tests + adversarial verification against the checked-out top of stack).
6ff8a75 to
a97a774
Compare
… test Follow-up to #5965 review (anton-107): - [low] NormalizeServerless did no validation, so malformed values (vv5, v, ' 5', 5x, latest) resolved to a bogus serverless-vN env key and only 404'd two phases later at fetch with a generic E_ENV_UNSUPPORTED. Add ValidServerlessVersion (bare number, or v-prefixed) and reject at the resolve phase with an actionable E_RESOLVE, per the repo's reject-early rule. - [low] The two default-serverless tests compared the code against the same constant it uses, so they'd pass for any value. Assert the concrete serverless/serverless-v5 key so the v5 default is genuinely pinned. Co-authored-by: Isaac
67c2589 to
e169ea6
Compare
|
Addressed — and thanks for catching the broken build; the root cause was worse than a stale assertion. [blocker] two tests fail — investigating this surfaced that a cascade-rebase earlier in the session had silently dropped this PR's main commit ("Accept bare serverless version numbers"), leaving only the test-assertion follow-up on the branch — which referenced the bare [low] tests were self-referential — fixed: both now assert the concrete [low] NormalizeServerless did no validation — fixed: added Fixes on the branch ( |
a97a774 to
57eb340
Compare
Per follow-up on the [P0] CLI Changes spec, --serverless-version is
documented to take a bare number (e.g. 5). NormalizeServerless already
accepts a bare number and remains tolerant of a v-prefix (v5/V5); both map
to the serverless-vN env key, so the environment-repo layout is unchanged.
- Flag help now shows 'e.g. 5'; the E_ENV_UNSUPPORTED hint suggests
'--serverless-version 5'.
- defaultServerlessVersion is stored in the bare form ('5'); it still
resolves to serverless-v5 via NormalizeServerless.
- Acceptance scripts pass the bare form; env-key goldens are unchanged
(still serverless-vN). Unit tests cover bare and v-prefixed input plus
the default constant.
Co-authored-by: Isaac
… test Follow-up to #5965 review (anton-107): - [low] NormalizeServerless did no validation, so malformed values (vv5, v, ' 5', 5x, latest) resolved to a bogus serverless-vN env key and only 404'd two phases later at fetch with a generic E_ENV_UNSUPPORTED. Add ValidServerlessVersion (bare number, or v-prefixed) and reject at the resolve phase with an actionable E_RESOLVE, per the repo's reject-early rule. - [low] The two default-serverless tests compared the code against the same constant it uses, so they'd pass for any value. Assert the concrete serverless/serverless-v5 key so the v5 default is genuinely pinned. Co-authored-by: Isaac
e169ea6 to
840642f
Compare
Stacked on #5964 → #5963 → #5962 → #5961 → #5960 → #5959.
What
--serverless-versionis now documented to take a bare number (e.g.5) rather thanv5.5is the documented form;v5/V5are still accepted (tolerant).serverless/serverless-vN, so the environments-repo layout is unchanged (stillserverless/serverless-v5/…).Changes
e.g. v4→e.g. 5; theE_ENV_UNSUPPORTEDhint suggests--serverless-version 5.defaultServerlessVersionstored in bare form ("5"); still resolves toserverless-v5viaNormalizeServerless.serverless-vN).Testing
go build ./..., lint (0 issues), deadcode clean, unit + acceptance green.This pull request and its description were written by Isaac.