Skip to content

Commit da87678

Browse files
maysunfaisalmaysunfaisal
committed
Review feedback: Update var name, nesting and test coverage
Signed-off-by: Maysun J Faisal <[email protected]>
1 parent 12f8991 commit da87678

File tree

9 files changed

+404
-226
lines changed

9 files changed

+404
-226
lines changed

‎pkg/validation/errors.go

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -121,24 +121,24 @@ func (e *MissingProjectRemoteError) Error() string {
121121
return fmt.Sprintf("project %s should have at least one remote", e.projectName)
122122
}
123123

124-
//MissingStarterProjectRemoteError returns an error if the git remotes object under a starterProject is empty
125-
type MissingStarterProjectRemoteError struct {
126-
objectName string
127-
projectName string
124+
//MissingRemoteError returns an error if the git remotes object is empty
125+
type MissingRemoteError struct {
126+
objectType string
127+
objectName string
128128
}
129129

130-
func (e *MissingStarterProjectRemoteError) Error() string {
131-
return fmt.Sprintf("%s %s should have at least one remote", e.objectName, e.projectName)
130+
func (e *MissingRemoteError) Error() string {
131+
return fmt.Sprintf("%s %s should have at least one remote", e.objectType, e.objectName)
132132
}
133133

134-
//MultipleStarterProjectRemoteError returns an error if multiple git remotes are specified. There can only be one remote.
135-
type MultipleStarterProjectRemoteError struct {
136-
objectName string
137-
projectName string
134+
//MultipleRemoteError returns an error if multiple git remotes are specified. There can only be one remote.
135+
type MultipleRemoteError struct {
136+
objectType string
137+
objectName string
138138
}
139139

140-
func (e *MultipleStarterProjectRemoteError) Error() string {
141-
return fmt.Sprintf("%s %s should have one remote only", e.objectName, e.projectName)
140+
func (e *MultipleRemoteError) Error() string {
141+
return fmt.Sprintf("%s %s should have one remote only", e.objectType, e.objectName)
142142
}
143143

144144
//MissingProjectCheckoutFromRemoteError returns an error if there are multiple git remotes but the checkoutFrom remote has not been specified
@@ -152,13 +152,13 @@ func (e *MissingProjectCheckoutFromRemoteError) Error() string {
152152

153153
//InvalidProjectCheckoutRemoteError returns an error if there is an unmatched, checkoutFrom remote specified
154154
type InvalidProjectCheckoutRemoteError struct {
155+
objectType string
155156
objectName string
156-
projectName string
157157
checkoutRemote string
158158
}
159159

160160
func (e *InvalidProjectCheckoutRemoteError) Error() string {
161-
return fmt.Sprintf("unable to find the checkout remote %s in the remotes for %s %s", e.checkoutRemote, e.objectName, e.projectName)
161+
return fmt.Sprintf("unable to find the checkout remote %s in the remotes for %s %s", e.checkoutRemote, e.objectType, e.objectName)
162162
}
163163

164164
// resolveErrorMessageWithImportAttributes returns an updated error message

‎pkg/validation/projects.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,27 +67,27 @@ func ValidateProjects(projects []v1alpha2.Project) (returnedErr error) {
6767
}
6868

6969
// validateRemoteMap checks if the checkout remote is present in the project remote map
70-
func validateRemoteMap(remotes map[string]string, checkoutRemote, object, name string) error {
70+
func validateRemoteMap(remotes map[string]string, checkoutRemote, objectType, objectName string) error {
7171

7272
if _, ok := remotes[checkoutRemote]; !ok {
7373

74-
return &InvalidProjectCheckoutRemoteError{projectName: name, objectName: object, checkoutRemote: checkoutRemote}
74+
return &InvalidProjectCheckoutRemoteError{objectName: objectName, objectType: objectType, checkoutRemote: checkoutRemote}
7575
}
7676

7777
return nil
7878
}
7979

8080
// validateSingleRemoteGitSrc validates a git src for a single remote only
81-
func validateSingleRemoteGitSrc(object, name string, gitSource v1alpha2.GitLikeProjectSource) (err error) {
81+
func validateSingleRemoteGitSrc(objectType, objectName string, gitSource v1alpha2.GitLikeProjectSource) (err error) {
8282
switch len(gitSource.Remotes) {
8383
case 0:
84-
err = &MissingStarterProjectRemoteError{objectName: object, projectName: name}
84+
err = &MissingRemoteError{objectType: objectType, objectName: objectName}
8585
case 1:
8686
if gitSource.CheckoutFrom != nil && gitSource.CheckoutFrom.Remote != "" {
87-
err = validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, object, name)
87+
err = validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, objectType, objectName)
8888
}
8989
default: // len(gitSource.Remotes) >= 2
90-
err = &MultipleStarterProjectRemoteError{objectName: object, projectName: name}
90+
err = &MultipleRemoteError{objectType: objectType, objectName: objectName}
9191
}
9292

9393
return err

‎pkg/validation/variables/test-fixtures/components/image-empty.yaml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
imageName: "myimage:xyz"

‎pkg/validation/variables/variables_command.go

Lines changed: 37 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -41,31 +41,33 @@ func ValidateAndReplaceForCommands(variables map[string]string, commands []v1alp
4141

4242
// validateAndReplaceForExecCommand validates the exec command data for global variable references and replaces them with the variable value
4343
func validateAndReplaceForExecCommand(variables map[string]string, exec *v1alpha2.ExecCommand) error {
44-
var err error
4544

45+
if exec == nil {
46+
return nil
47+
}
48+
49+
var err error
4650
invalidKeys := make(map[string]bool)
4751

48-
if exec != nil {
49-
// Validate exec command line
50-
if exec.CommandLine, err = validateAndReplaceDataWithVariable(exec.CommandLine, variables); err != nil {
51-
checkForInvalidError(invalidKeys, err)
52-
}
52+
// Validate exec command line
53+
if exec.CommandLine, err = validateAndReplaceDataWithVariable(exec.CommandLine, variables); err != nil {
54+
checkForInvalidError(invalidKeys, err)
55+
}
5356

54-
// Validate exec working dir
55-
if exec.WorkingDir, err = validateAndReplaceDataWithVariable(exec.WorkingDir, variables); err != nil {
56-
checkForInvalidError(invalidKeys, err)
57-
}
57+
// Validate exec working dir
58+
if exec.WorkingDir, err = validateAndReplaceDataWithVariable(exec.WorkingDir, variables); err != nil {
59+
checkForInvalidError(invalidKeys, err)
60+
}
5861

59-
// Validate exec label
60-
if exec.Label, err = validateAndReplaceDataWithVariable(exec.Label, variables); err != nil {
61-
checkForInvalidError(invalidKeys, err)
62-
}
62+
// Validate exec label
63+
if exec.Label, err = validateAndReplaceDataWithVariable(exec.Label, variables); err != nil {
64+
checkForInvalidError(invalidKeys, err)
65+
}
6366

64-
// Validate exec env
65-
if len(exec.Env) > 0 {
66-
if err = validateAndReplaceForEnv(variables, exec.Env); err != nil {
67-
checkForInvalidError(invalidKeys, err)
68-
}
67+
// Validate exec env
68+
if len(exec.Env) > 0 {
69+
if err = validateAndReplaceForEnv(variables, exec.Env); err != nil {
70+
checkForInvalidError(invalidKeys, err)
6971
}
7072
}
7173

@@ -74,31 +76,35 @@ func validateAndReplaceForExecCommand(variables map[string]string, exec *v1alpha
7476

7577
// validateAndReplaceForCompositeCommand validates the composite command data for global variable references and replaces them with the variable value
7678
func validateAndReplaceForCompositeCommand(variables map[string]string, composite *v1alpha2.CompositeCommand) error {
77-
var err error
7879

80+
if composite == nil {
81+
return nil
82+
}
83+
84+
var err error
7985
invalidKeys := make(map[string]bool)
8086

81-
if composite != nil {
82-
// Validate composite label
83-
if composite.Label, err = validateAndReplaceDataWithVariable(composite.Label, variables); err != nil {
84-
checkForInvalidError(invalidKeys, err)
85-
}
87+
// Validate composite label
88+
if composite.Label, err = validateAndReplaceDataWithVariable(composite.Label, variables); err != nil {
89+
checkForInvalidError(invalidKeys, err)
8690
}
8791

8892
return newInvalidKeysError(invalidKeys)
8993
}
9094

9195
// validateAndReplaceForApplyCommand validates the apply command data for global variable references and replaces them with the variable value
9296
func validateAndReplaceForApplyCommand(variables map[string]string, apply *v1alpha2.ApplyCommand) error {
93-
var err error
9497

98+
if apply == nil {
99+
return nil
100+
}
101+
102+
var err error
95103
invalidKeys := make(map[string]bool)
96104

97-
if apply != nil {
98-
// Validate apply label
99-
if apply.Label, err = validateAndReplaceDataWithVariable(apply.Label, variables); err != nil {
100-
checkForInvalidError(invalidKeys, err)
101-
}
105+
// Validate apply label
106+
if apply.Label, err = validateAndReplaceDataWithVariable(apply.Label, variables); err != nil {
107+
checkForInvalidError(invalidKeys, err)
102108
}
103109

104110
return newInvalidKeysError(invalidKeys)

‎pkg/validation/variables/variables_command_test.go

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package variables
22

33
import (
4+
"reflect"
45
"testing"
56

67
"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
@@ -30,6 +31,13 @@ func TestValidateAndReplaceExecCommand(t *testing.T) {
3031
variableFile: "test-fixtures/variables/variables-notreferenced.yaml",
3132
wantErr: true,
3233
},
34+
{
35+
name: "Not an exec command",
36+
testFile: "test-fixtures/components/volume.yaml",
37+
outputFile: "test-fixtures/components/volume.yaml",
38+
variableFile: "test-fixtures/variables/variables-notreferenced.yaml",
39+
wantErr: false,
40+
},
3341
}
3442
for _, tt := range tests {
3543
t.Run(tt.name, func(t *testing.T) {
@@ -39,7 +47,12 @@ func TestValidateAndReplaceExecCommand(t *testing.T) {
3947
testVariable := make(map[string]string)
4048
readFileToStruct(t, tt.variableFile, &testVariable)
4149

42-
err := validateAndReplaceForExecCommand(testVariable, &testExecCommand)
50+
var err error
51+
if reflect.DeepEqual(testExecCommand, v1alpha2.ExecCommand{}) {
52+
err = validateAndReplaceForExecCommand(testVariable, nil)
53+
} else {
54+
err = validateAndReplaceForExecCommand(testVariable, &testExecCommand)
55+
}
4356
_, ok := err.(*InvalidKeysError)
4457
if tt.wantErr && !ok {
4558
t.Errorf("Expected InvalidKeysError error from test but got %+v", err)
@@ -77,6 +90,13 @@ func TestValidateAndReplaceCompositeCommand(t *testing.T) {
7790
variableFile: "test-fixtures/variables/variables-notreferenced.yaml",
7891
wantErr: true,
7992
},
93+
{
94+
name: "Not a composite command",
95+
testFile: "test-fixtures/components/volume.yaml",
96+
outputFile: "test-fixtures/components/volume.yaml",
97+
variableFile: "test-fixtures/variables/variables-notreferenced.yaml",
98+
wantErr: false,
99+
},
80100
}
81101
for _, tt := range tests {
82102
t.Run(tt.name, func(t *testing.T) {
@@ -86,7 +106,12 @@ func TestValidateAndReplaceCompositeCommand(t *testing.T) {
86106
testVariable := make(map[string]string)
87107
readFileToStruct(t, tt.variableFile, &testVariable)
88108

89-
err := validateAndReplaceForCompositeCommand(testVariable, &testCompositeCommand)
109+
var err error
110+
if reflect.DeepEqual(testCompositeCommand, v1alpha2.CompositeCommand{}) {
111+
err = validateAndReplaceForCompositeCommand(testVariable, nil)
112+
} else {
113+
err = validateAndReplaceForCompositeCommand(testVariable, &testCompositeCommand)
114+
}
90115
_, ok := err.(*InvalidKeysError)
91116
if tt.wantErr && !ok {
92117
t.Errorf("Expected InvalidKeysError error from test but got %+v", err)
@@ -124,6 +149,13 @@ func TestValidateAndReplaceApplyCommand(t *testing.T) {
124149
variableFile: "test-fixtures/variables/variables-notreferenced.yaml",
125150
wantErr: true,
126151
},
152+
{
153+
name: "Not an apply command",
154+
testFile: "test-fixtures/components/volume.yaml",
155+
outputFile: "test-fixtures/components/volume.yaml",
156+
variableFile: "test-fixtures/variables/variables-notreferenced.yaml",
157+
wantErr: false,
158+
},
127159
}
128160
for _, tt := range tests {
129161
t.Run(tt.name, func(t *testing.T) {
@@ -133,7 +165,12 @@ func TestValidateAndReplaceApplyCommand(t *testing.T) {
133165
testVariable := make(map[string]string)
134166
readFileToStruct(t, tt.variableFile, &testVariable)
135167

136-
err := validateAndReplaceForApplyCommand(testVariable, &testApplyCommand)
168+
var err error
169+
if reflect.DeepEqual(testApplyCommand, v1alpha2.ApplyCommand{}) {
170+
err = validateAndReplaceForApplyCommand(testVariable, nil)
171+
} else {
172+
err = validateAndReplaceForApplyCommand(testVariable, &testApplyCommand)
173+
}
137174
_, ok := err.(*InvalidKeysError)
138175
if tt.wantErr && !ok {
139176
t.Errorf("Expected InvalidKeysError error from test but got %+v", err)

0 commit comments

Comments
 (0)