-
Notifications
You must be signed in to change notification settings - Fork 194
[VPEX][1/8] Add local-env foundation: result types and env-key mapping #5823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
22f99d9
Add local-env foundation: result types and env-key mapping
rugpanov 22c2902
Fix PythonMinorFromRequires to pick the lower bound in multi-clause s…
rugpanov 5b0e0cc
Reject requires-python with no lower bound instead of picking a forbi…
rugpanov 7af0d48
Parse requires-python by clause and install the effective (highest) l…
rugpanov be61b7d
Treat a strict ">" python bound as excluding the whole minor series
rugpanov 900518b
Address review: fix patch-qualified strict python bound; guarantee --…
rugpanov afca77c
Merge branch 'main' into dbconnect/01-engine
rugpanov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| package localenv | ||
|
|
||
| import ( | ||
| "fmt" | ||
| "regexp" | ||
| "strconv" | ||
| "strings" | ||
| ) | ||
|
|
||
| // clauseRe splits a single requires-python clause into its operator (optional), | ||
| // MAJOR.MINOR version, and an optional patch component. A clause with no operator | ||
| // is a bare floor. The patch capture (group 4) is needed to interpret a strict | ||
| // ">" correctly: ">3.10" excludes all of 3.10.x, but ">3.10.5" is still satisfied | ||
| // by 3.10.6, so only the former bumps the minor. | ||
| var clauseRe = regexp.MustCompile(`^(>=|<=|===|==|~=|!=|<|>)?\s*(\d+)\.(\d+)(\.\d+)?`) | ||
|
|
||
| // NormalizeServerless returns the canonical "vN" spelling of a serverless | ||
| // version accepting "4", "v4", or "V4". | ||
| func NormalizeServerless(version string) string { | ||
| return "v" + strings.TrimPrefix(strings.ToLower(version), "v") | ||
| } | ||
|
|
||
| // EnvKeyForServerless returns the environment key for a serverless version. | ||
| func EnvKeyForServerless(version string) string { | ||
| return "serverless/serverless-" + NormalizeServerless(version) | ||
| } | ||
|
|
||
| // EnvKeyForSparkVersion returns the environment key for a Spark version. | ||
| func EnvKeyForSparkVersion(sparkVersion string) string { | ||
| return "dbr/" + sparkVersion | ||
| } | ||
|
|
||
| // PythonMinorFromRequires parses a PEP 440 requires-python string and returns | ||
| // the MAJOR.MINOR of the Python version to install: the effective lower bound. | ||
| // | ||
| // A requires-python is a comma-separated list of clauses in any order (e.g. | ||
| // "<3.13,>=3.10"). Each clause is classified by operator: | ||
| // - lower-bound / pinning (>=, >, ==, ~=, ===) or a bare MAJOR.MINOR with no | ||
| // operator establishes a floor; | ||
| // - upper-bound / exclusion (<, <=, !=) does not — those versions are capped | ||
| // or forbidden and must never be installed. | ||
| // | ||
| // The result is the highest floor across all floor clauses (so ">=3.8,>=3.11" | ||
| // yields 3.11, the version that satisfies every clause). A spec with no floor | ||
| // clause at all (e.g. "<3.13" or "!=3.12") is an error rather than a guess. | ||
| func PythonMinorFromRequires(requiresPython string) (string, error) { | ||
| bestMajor, bestMinor := -1, -1 | ||
| sawClause := false | ||
| for clause := range strings.SplitSeq(requiresPython, ",") { | ||
| clause = strings.TrimSpace(clause) | ||
| if clause == "" { | ||
| continue | ||
| } | ||
| m := clauseRe.FindStringSubmatch(clause) | ||
| if m == nil { | ||
| continue | ||
| } | ||
| sawClause = true | ||
| op := m[1] | ||
| // Upper-bound and exclusion operators never establish a floor. | ||
| if op == "<" || op == "<=" || op == "!=" { | ||
| continue | ||
| } | ||
| major, _ := strconv.Atoi(m[2]) | ||
| minor, _ := strconv.Atoi(m[3]) | ||
| hasPatch := m[4] != "" | ||
| // A strict ">" with no patch excludes the whole given minor series (PEP 440: | ||
| // ">3.10" matches neither 3.10 nor any 3.10.x), so the lowest installable | ||
| // minor is the next one up. But ">3.10.5" is still satisfied by 3.10.6, so a | ||
| // patch-qualified strict bound leaves the minor unchanged. | ||
| if op == ">" && !hasPatch { | ||
| minor++ | ||
| } | ||
| if major > bestMajor || (major == bestMajor && minor > bestMinor) { | ||
| bestMajor, bestMinor = major, minor | ||
| } | ||
| } | ||
| if bestMajor >= 0 { | ||
| return fmt.Sprintf("%d.%d", bestMajor, bestMinor), nil | ||
| } | ||
| if sawClause { | ||
| return "", fmt.Errorf("requires-python %q has no lower bound to install from", requiresPython) | ||
| } | ||
| return "", fmt.Errorf("cannot parse python version from %q", requiresPython) | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| package localenv | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestEnvKeyForServerless(t *testing.T) { | ||
| for _, in := range []string{"4", "v4", "V4"} { | ||
| assert.Equal(t, "serverless/serverless-v4", EnvKeyForServerless(in)) | ||
| } | ||
| } | ||
|
|
||
| func TestEnvKeyForSparkVersion(t *testing.T) { | ||
| assert.Equal(t, "dbr/15.4.x-scala2.12", EnvKeyForSparkVersion("15.4.x-scala2.12")) | ||
| } | ||
|
|
||
| func TestPythonMinorFromRequires(t *testing.T) { | ||
| cases := map[string]string{ | ||
| "==3.12.*": "3.12", | ||
| ">=3.12": "3.12", | ||
| "==3.12.3": "3.12", | ||
| "~=3.11": "3.11", | ||
| // Multi-clause specifiers: the lower bound is the version to install, | ||
| // regardless of clause order. Taking the first number would pick the | ||
| // excluded upper bound (e.g. 3.13 from "<3.13"). | ||
| "<3.13,>=3.10": "3.10", | ||
| ">=3.10,<3.13": "3.10", | ||
| ">=3.10, <3.13": "3.10", | ||
| "<4.0,>=3.9": "3.9", | ||
| "===3.11": "3.11", | ||
| // The effective floor is the HIGHEST lower bound, regardless of order. | ||
| ">=3.8,>=3.11": "3.11", | ||
| ">=3.11,>=3.8": "3.11", | ||
| // A bare floor alongside an exclusion is still a floor. | ||
| "!=3.11,3.12": "3.12", | ||
| "3.12,!=3.12.4": "3.12", | ||
| // Bare version with no operator. | ||
| "3.12": "3.12", | ||
| // Whitespace and patch components tolerated. | ||
| ">= 3.10 , < 3.13": "3.10", | ||
| // Strict ">" with no patch excludes the whole minor series (PEP 440), so | ||
| // the floor is the next minor up. | ||
| ">3.10": "3.11", | ||
| ">3.10,<3.13": "3.11", | ||
| ">=3.9,>3.10": "3.11", | ||
| // Strict ">" WITH a patch does not exclude the minor series: 3.10.6 | ||
| // satisfies ">3.10.5", so the floor stays 3.10. | ||
| ">3.10.5": "3.10", | ||
| ">3.10.5,<3.13": "3.10", | ||
| ">=3.10.2": "3.10", | ||
| } | ||
| for in, want := range cases { | ||
| got, err := PythonMinorFromRequires(in) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, want, got, "input %q", in) | ||
| } | ||
|
|
||
| // No usable floor: only upper-bound / exclusion clauses. Must error rather | ||
| // than select a forbidden/capped version. | ||
| for _, in := range []string{"<3.13", "<=3.12", "!=3.12", "<3.13,!=3.12", "garbage", ""} { | ||
| _, err := PythonMinorFromRequires(in) | ||
| assert.Error(t, err, "input %q must error", in) | ||
| } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,195 @@ | ||
| package localenv | ||
|
|
||
| import "fmt" | ||
|
|
||
| // Command path components, defined once so a rename touches a single place | ||
| // (spec §0 / invariant 8 / scenario 21). The cmd layer builds the Cobra | ||
| // command tree from CommandGroup/CommandSubgroup/CommandVerb; the --json | ||
| // "command" field uses CommandName. No other string re-spells the command path. | ||
| const ( | ||
| CommandGroup = "local-env" | ||
| CommandSubgroup = "python" | ||
| CommandVerb = "sync" | ||
| CommandName = CommandGroup + " " + CommandSubgroup + " " + CommandVerb | ||
|
|
||
| // SchemaVersion is the version of the --json output contract (spec §6). | ||
| // Bump it on any breaking change to the JSON shape. | ||
| SchemaVersion = 1 | ||
| ) | ||
|
|
||
| // Mode is the provisioning mode: a full environment (default) or the | ||
| // constraints-only variant that omits the databricks-connect dependency. | ||
| type Mode int | ||
|
|
||
| const ( | ||
| ModeDefault Mode = iota | ||
| ModeConstraintsOnly | ||
| ) | ||
|
|
||
| // String returns the JSON/text spelling of the mode ("default" | "constraints-only"). | ||
| func (m Mode) String() string { | ||
| if m == ModeConstraintsOnly { | ||
| return "constraints-only" | ||
| } | ||
| return "default" | ||
| } | ||
|
|
||
| // PhaseName is a canonical execution phase (spec §3 / §6). The set is fixed and | ||
| // ordered; the --json "phases" array reports every phase in this order. | ||
| type PhaseName string | ||
|
|
||
| const ( | ||
| PhasePreflight PhaseName = "preflight" | ||
| PhaseResolve PhaseName = "resolve" | ||
| PhaseFetch PhaseName = "fetch" | ||
| PhaseMerge PhaseName = "merge" | ||
| PhaseProvision PhaseName = "provision" | ||
| PhaseValidate PhaseName = "validate" | ||
| ) | ||
|
|
||
| // Phase status values (spec §6.2). | ||
| const ( | ||
| StatusOK = "ok" | ||
| StatusError = "error" | ||
| StatusPending = "pending" | ||
| ) | ||
|
|
||
| // ErrorCode is a stable failure-class identifier surfaced in --json error.code | ||
| // (spec §7). Values are compared via the ErrorCode constants, never by | ||
| // string-matching messages, and are defined once here. | ||
| type ErrorCode string | ||
|
|
||
| const ( | ||
| ErrNoTarget ErrorCode = "E_NO_TARGET" | ||
| ErrManagerUnsupported ErrorCode = "E_MANAGER_UNSUPPORTED" | ||
| ErrUvMissing ErrorCode = "E_UV_MISSING" | ||
| ErrNotWritable ErrorCode = "E_NOT_WRITABLE" | ||
| ErrResolve ErrorCode = "E_RESOLVE" | ||
| ErrEnvUnsupported ErrorCode = "E_ENV_UNSUPPORTED" | ||
| ErrFetch ErrorCode = "E_FETCH" | ||
| ErrWrite ErrorCode = "E_WRITE" | ||
| ErrMerge ErrorCode = "E_MERGE" | ||
| ErrPythonInstall ErrorCode = "E_PYTHON_INSTALL" | ||
| ErrProvision ErrorCode = "E_PROVISION" | ||
| ErrValidate ErrorCode = "E_VALIDATE" | ||
| ) | ||
|
|
||
| // PipelineError is a failure carrying a stable code, the phase at which it | ||
| // occurred, and whether disk was mutated before the failure. It marshals to the | ||
| // --json error object (spec §6.2). Code and FailurePhase are the stable | ||
| // contract; Err holds the wrapped cause for errors.Is/As and is not serialized. | ||
| type PipelineError struct { | ||
| Code ErrorCode `json:"code"` | ||
| FailurePhase PhaseName `json:"failurePhase"` | ||
| Msg string `json:"message"` | ||
| DiskMutated bool `json:"diskMutated"` | ||
| Err error `json:"-"` | ||
| } | ||
|
|
||
| func (e *PipelineError) Error() string { | ||
| if e.Err != nil { | ||
| return e.Msg + ": " + e.Err.Error() | ||
| } | ||
| return e.Msg | ||
| } | ||
|
|
||
| func (e *PipelineError) Unwrap() error { | ||
| return e.Err | ||
| } | ||
|
|
||
| // NewError creates a PipelineError with a code and message. FailurePhase and | ||
| // DiskMutated are filled in by the pipeline when it records the failure. The | ||
| // message is formatted with fmt.Sprintf(format, args...); err may be nil. | ||
| func NewError(code ErrorCode, err error, format string, args ...any) *PipelineError { | ||
| return &PipelineError{ | ||
| Code: code, | ||
| Msg: fmt.Sprintf(format, args...), | ||
| Err: err, | ||
| } | ||
| } | ||
|
|
||
| // TargetInfo is the resolved compute target (spec §6 "target"). Source records | ||
| // which of the four precedence sources was used. SparkVersion is the raw cluster | ||
| // runtime string the resolver read; it is folded into EnvKey (dbr/<SparkVersion>) | ||
| // and is not part of the JSON contract, kept only as intermediate resolver state. | ||
| type TargetInfo struct { | ||
| Source string `json:"source"` | ||
| ClusterID string `json:"clusterId,omitempty"` | ||
| ServerlessVersion string `json:"serverlessVersion,omitempty"` | ||
| EnvKey string `json:"envKey"` | ||
|
|
||
| SparkVersion string `json:"-"` | ||
| } | ||
|
|
||
| // ResolvedInfo is the resolved environment definition (spec §6 "resolved"). | ||
| // DBConnectVersion is omitted in constraints-only mode. | ||
| type ResolvedInfo struct { | ||
| PythonVersion string `json:"pythonVersion"` | ||
| DBConnectVersion string `json:"dbconnectVersion,omitempty"` | ||
| ArtifactSource string `json:"artifactSource"` | ||
| } | ||
|
|
||
| // Plan describes the changes a --check run would apply (spec §6.3). | ||
| // ChangedRegions is retained for text output only and is not serialized. | ||
| type Plan struct { | ||
| WouldWrite string `json:"wouldWrite"` | ||
| WouldBackup string `json:"wouldBackup,omitempty"` | ||
| WouldInstallPython string `json:"wouldInstallPython,omitempty"` | ||
| Diff string `json:"diff"` | ||
|
|
||
| ChangedRegions []string `json:"-"` | ||
| } | ||
|
|
||
| // PhaseStatus is one entry in the --json "phases" array (spec §6). Detail is | ||
| // used for human-readable text output only and is not serialized. | ||
| type PhaseStatus struct { | ||
| Phase PhaseName `json:"phase"` | ||
| Status string `json:"status"` | ||
|
|
||
| Detail string `json:"-"` | ||
| } | ||
|
|
||
| // Warning is a non-fatal advisory surfaced in --json "warnings" (spec §6). | ||
| type Warning struct { | ||
| Code string `json:"code"` | ||
| Message string `json:"message"` | ||
| } | ||
|
|
||
| // Result is the full outcome of a sync run and the root of the --json object | ||
| // (spec §6). Field order matches the spec's schema so JSON key order is stable. | ||
| // | ||
| // Phases and Warnings are non-omitempty slices, so they must always be non-nil | ||
| // before marshalling or the --json contract would emit "null" instead of "[]" — | ||
| // a distinction that trips JSON consumers and golden diffs. Construct a Result | ||
| // with NewResult (or otherwise seed both) rather than a bare Result{} literal. | ||
| type Result struct { | ||
| SchemaVersion int `json:"schemaVersion"` | ||
| Command string `json:"command"` | ||
| OK bool `json:"ok"` | ||
| Mode string `json:"mode"` | ||
| DryRun bool `json:"dryRun"` | ||
| Target *TargetInfo `json:"target,omitempty"` | ||
| Resolved *ResolvedInfo `json:"resolved,omitempty"` | ||
| Greenfield bool `json:"greenfield"` | ||
| Plan *Plan `json:"plan,omitempty"` | ||
| VenvPath string `json:"venvPath,omitempty"` | ||
| Phases []PhaseStatus `json:"phases"` | ||
| Warnings []Warning `json:"warnings"` | ||
| Error *PipelineError `json:"error"` | ||
| BackupPath string `json:"backupPath,omitempty"` | ||
| // DurationMs is part of the §6 contract but reserved for now: the pipeline | ||
| // does not measure wall time (a real clock would make acceptance goldens | ||
| // non-deterministic), so it is always emitted as 0 until timing is wired | ||
| // through a clock the tests can control. | ||
| DurationMs int64 `json:"durationMs"` | ||
| } | ||
|
|
||
| // NewResult returns a Result with the non-omitempty slice fields initialized to | ||
| // empty (non-nil) slices, so the --json output always renders "phases": [] and | ||
| // "warnings": [] rather than "null". Callers fill in the remaining fields. | ||
| func NewResult() *Result { | ||
| return &Result{ | ||
| Phases: []PhaseStatus{}, | ||
| Warnings: []Warning{}, | ||
| } | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| package localenv | ||
|
|
||
| import ( | ||
| "encoding/json" | ||
| "errors" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestPipelineErrorWrapsAndExposesCode(t *testing.T) { | ||
| base := errors.New("boom") | ||
| err := NewError(ErrFetch, base, "fetch %s", "x") | ||
| assert.Equal(t, "fetch x: boom", err.Error()) | ||
| assert.Equal(t, ErrFetch, err.Code) | ||
| assert.ErrorIs(t, err, base) | ||
| } | ||
|
|
||
| func TestModeString(t *testing.T) { | ||
| assert.Equal(t, "default", ModeDefault.String()) | ||
| assert.Equal(t, "constraints-only", ModeConstraintsOnly.String()) | ||
| } | ||
|
|
||
| func TestCommandName(t *testing.T) { | ||
| // The --json "command" field and all help text derive from these; the | ||
| // three-part path must join to the full command a user types. | ||
| assert.Equal(t, "local-env python sync", CommandName) | ||
| } | ||
|
|
||
| func TestNewResultEmitsEmptyArraysNotNull(t *testing.T) { | ||
| // The --json contract requires phases/warnings to render as [] not null; | ||
| // NewResult seeds them so consumers and golden diffs see a stable shape. | ||
| b, err := json.Marshal(NewResult()) | ||
| require.NoError(t, err) | ||
| s := string(b) | ||
| assert.Contains(t, s, `"phases":[]`) | ||
| assert.Contains(t, s, `"warnings":[]`) | ||
| assert.NotContains(t, s, `"phases":null`) | ||
| assert.NotContains(t, s, `"warnings":null`) | ||
| // A bare Result{} literal is the shape NewResult exists to avoid. | ||
| bare, err := json.Marshal(&Result{}) | ||
| require.NoError(t, err) | ||
| assert.Contains(t, string(bare), `"phases":null`, "sanity: bare literal is the null case") | ||
| } |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PhasesandWarningshave noomitemptyand are slices, so a zero-valueResultmarshals them as"phases": null, "warnings": nullrather than[]. For a versioned--jsoncontract,null-vs-empty-array is a classic ambiguity that trips up consumers (and golden-file diffs).If the pipeline (#5828) always initializes both to non-nil before marshalling, this is moot — could you confirm that here, or otherwise guarantee they always emit as
[]?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and hardened. The pipeline (#5828) always seeds both to non-nil (
initialPhases()and[]Warning{}), so--jsonnever emitsnullin practice. To guarantee it at the type level rather than by convention, I added aNewResult()constructor here that seeds both slices, a doc note on the invariant, and a test asserting the JSON emits[](and that a bareResult{}is the null case to avoid). #5828 now constructs its Result viaNewResult(). (commit 900518b)