Tool 1651 validate version #696
Conversation
ghost
requested review from
models/models.go
Outdated
| } | ||
|
|
||
| func (stepIDData StepIDData) validate(defaultStepLibSource string) (err error) { | ||
| if !stepIDData.isLatest() && |
There was a problem hiding this comment.
I recommend to inline the isLatest function.
From the perspective of the validate function, the rule is not to "validate version string if not latest", but to validate it "if not empty".
There is no added value in the context of the validate function if we know, that an empty version string resolves to the latest version.
models/models.go
Outdated
| @@ -104,6 +104,15 @@ type StepIDData struct { | |||
| Version string | |||
| } | |||
|
|
|||
| func (stepIDData StepIDData) validate(defaultStepLibSource string) (err error) { | |||
There was a problem hiding this comment.
pls apply the version validation for every step from any stepLib
models/models_methods.go
Outdated
| @@ -288,6 +288,15 @@ func (workflow *WorkflowModel) Validate() ([]string, error) { | |||
| return warnings, err | |||
| } | |||
|
|
|||
| stepIDData, err := CreateStepIDDataFromString(stepID, defaultStepLibSource) | |||
There was a problem hiding this comment.
pls solve to replace this function, as the new input (defaultStepLibSource), is only needed for using the CreateStepIDDataFromString, but we do not need the StepSource information otherwise
models/models_methods.go
Outdated
| } | ||
|
|
||
| if err := stepIDData.validate(defaultStepLibSource); err != nil { | ||
| return warnings, fmt.Errorf("invalid version format (%s) specified for step ID: %s", stepIDData.Version, stepIDData.IDorURI) |
There was a problem hiding this comment.
it is a generic stepIDData validator, we can not be sure that only the version is wrong
models/models_methods_test.go
Outdated
| } | ||
| for _, tt := range []struct { | ||
| composite string | ||
| stepSrc string |
There was a problem hiding this comment.
stepSrc should be defaultSteplibSource
models/models_methods_test.go
Outdated
| { | ||
| composite: "steplib-src::[email protected]", stepSrc: "", | ||
| wantStepSrc: "steplib-src", wantVersion: "0.0.1", wantStepID: "step-id", wantErr: false, | ||
| name: "default / long / verbose ID mode", |
There was a problem hiding this comment.
name should be the first as it describes the test case, so it lets you understand the following test case properties.
models/models_methods_test.go
Outdated
| }{ | ||
| { | ||
| composite: "steplib-src::[email protected]", stepSrc: "", | ||
| wantStepSrc: "steplib-src", wantVersion: "0.0.1", wantStepID: "step-id", wantErr: false, |
There was a problem hiding this comment.
if you change the order of wantStepID and wantVersion it is easiert toreview (as it composite has a format: src::id@version)
There was a problem hiding this comment.
suggested structure:
name: "default / long / verbose ID mode",
composite: "steplib-src::[email protected]", defaultStepLib: "",
wantStepSrc: "steplib-src", wantStepID: "step-id", wantVersion: "0.0.1",
wantErr: false,
models/models_methods_test.go
Outdated
| name: "no steplib & no version, only step-id", | ||
| }, | ||
| { | ||
| composite: "", stepSrc: "def-step-src", |
There was a problem hiding this comment.
clear def-step-src and unify it, sometimes you use: def-step-src, sometimes: default-steplib-src, use a short stepSrc, like (StepLib)
models/models_methods_test.go
Outdated
| name: "special empty test", | ||
| }, | ||
| { | ||
| composite: "path::/some/path", stepSrc: "", |
There was a problem hiding this comment.
move the different sources into different test (something like TestCreateStepIDDataFromString_steplib, TestCreateStepIDDataFromString_path, TestCreateStepIDDataFromString_git, TestCreateStepIDDataFromString_legacy)
models/models_methods.go
Outdated
| @@ -288,6 +288,14 @@ func (workflow *WorkflowModel) Validate() ([]string, error) { | |||
| return warnings, err | |||
| } | |||
|
|
|||
| stepNode := stepNode(stepID) | |||
|
|
|||
| if ver, src := stepNode.version(), stepNode.source(); len(ver) > 0 && src == stepSourceUnknown { | |||
There was a problem hiding this comment.
src == stepSourceUnknown or any step library
models/models_methods.go
Outdated
| stepSourceStepLibIndependent stepSource = "_" | ||
| ) | ||
|
|
||
| func (sn stepNode) source() stepSource { |
There was a problem hiding this comment.
could you please comment 1-1 examples to the below functions to make the review easier
models/models_methods.go
Outdated
| composite := sn.composite() | ||
|
|
||
| s := strings.Split(composite, "@") | ||
| if item := s[0]; item == "git" { |
There was a problem hiding this comment.
would be great to resolve code duplications
models/models_methods.go
Outdated
| @@ -458,7 +464,7 @@ func (config *BitriseDataModel) Validate() ([]string, error) { | |||
| warns, err := workflow.Validate() | |||
| warnings = append(warnings, warns...) | |||
| if err != nil { | |||
| return warnings, err | |||
| return warnings, fmt.Errorf("validation error in workflow: %s, error: %s", ID, err) | |||
There was a problem hiding this comment.
avoid adding , error: suffix, the idiomatic error looks like: reason: deeper reason: even deeper reason
models/models_methods_test.go
Outdated
| @@ -1580,3 +1535,296 @@ workflows: | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| func Test_stepNode(t *testing.T) { | |||
There was a problem hiding this comment.
i suggest to drop implementation details tests
bitrise-io/stepman#287