Skip to content

Commit

Permalink
Address comments
Browse files Browse the repository at this point in the history
  • Loading branch information
iamhopaul123 committed Sep 10, 2021
1 parent dc44317 commit 62c7c91
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 21 deletions.
3 changes: 1 addition & 2 deletions internal/pkg/manifest/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ package manifest

import (
"fmt"
"strings"

"github.com/dustin/go-humanize/english"
)
Expand Down Expand Up @@ -64,7 +63,7 @@ func (e *errFieldMustBeSpecified) Error() string {
if len(e.conditionalFields) == 0 {
return errMsg
}
return fmt.Sprintf(`%s if "%s" %s specified`, errMsg, strings.Join(e.conditionalFields, `", "`),
return fmt.Sprintf(`%s if "%s" %s specified`, errMsg, english.WordSeries(e.conditionalFields, "or"),
english.PluralWord(len(e.conditionalFields), "is", "are"))
}

Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/manifest/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
)

var (
errUnmarshalEFSOpts = errors.New(`cannot unmarshal efs field into bool or map`)
errUnmarshalEFSOpts = errors.New(`cannot unmarshal "efs" field into bool or map`)
)

// Storage represents the options for external and native storage.
Expand Down
14 changes: 3 additions & 11 deletions internal/pkg/manifest/svc.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ var ServiceTypes = []string{
WorkerServiceType,
}

// Range contains either a Range or a range configuration for Autoscaling ranges
// Range contains either a Range or a range configuration for Autoscaling ranges.
type Range struct {
Value *IntRangeBand // Mutually exclusive with RangeConfig
RangeConfig RangeConfig
Expand All @@ -44,20 +44,12 @@ func (r *Range) IsEmpty() bool {
return r.Value == nil && r.RangeConfig.IsEmpty()
}

// Parse extracts the min and max from RangeOpts
// Parse extracts the min and max from RangeOpts.
func (r *Range) Parse() (min int, max int, err error) {
if r.Value != nil && !r.RangeConfig.IsEmpty() {
return 0, 0, &errFieldMutualExclusive{
firstField: "range",
secondField: "min/max",
}
}

if r.Value != nil {
return r.Value.Parse()
}

return *r.RangeConfig.Min, *r.RangeConfig.Max, nil
return aws.IntValue(r.RangeConfig.Min), aws.IntValue(r.RangeConfig.Max), nil
}

// UnmarshalYAML overrides the default YAML unmarshaling logic for the RangeOpts
Expand Down
24 changes: 19 additions & 5 deletions internal/pkg/manifest/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,10 @@ func (i *Image) Validate() error {

// Validate returns if BuildArgsOrString is configured correctly.
func (b *BuildArgsOrString) Validate() error {
return b.BuildArgs.Validate()
if !b.BuildArgs.isEmpty() {
return b.BuildArgs.Validate()
}
return nil
}

// Validate returns if DockerBuildArgs is configured correctly.
Expand Down Expand Up @@ -221,8 +224,7 @@ func (a *AdvancedCount) Validate() error {
if err := a.Range.Validate(); err != nil {
return fmt.Errorf(`validate "range": %w`, err)
}
}
if a.CPU != nil || a.Memory != nil || a.Requests != nil || a.ResponseTime != nil {
} else if a.CPU != nil || a.Memory != nil || a.Requests != nil || a.ResponseTime != nil {
return &errFieldMustBeSpecified{
missingField: "range",
conditionalFields: []string{"cpu_percentage", "memory_percentage", "requests", "response_time"},
Expand Down Expand Up @@ -267,6 +269,18 @@ func (r *IntRangeBand) Validate() error {

// Validate returns if RangeConfig is configured correctly.
func (r *RangeConfig) Validate() error {
if r.Min == nil && r.Max != nil {
return &errFieldMustBeSpecified{
missingField: "min",
conditionalFields: []string{"max"},
}
}
if r.Min != nil && r.Max == nil {
return &errFieldMustBeSpecified{
missingField: "max",
conditionalFields: []string{"min"},
}
}
min, max := aws.IntValue(r.Min), aws.IntValue(r.Max)
if min <= max {
return nil
Expand Down Expand Up @@ -344,12 +358,12 @@ func (e *EFSVolumeConfiguration) Validate() error {
if err := e.AuthConfig.Validate(); err != nil {
return fmt.Errorf(`validate "auth": %w`, err)
}
if aws.StringValue(e.AuthConfig.AccessPointID) != "" {
if e.AuthConfig.AccessPointID != nil {
if (aws.StringValue(e.RootDirectory) == "" || aws.StringValue(e.RootDirectory) == "/") &&
aws.BoolValue(e.AuthConfig.IAM) {
return nil
}
return fmt.Errorf("root_dir must be either empty or / and auth.iam must be true when access_point_id is in used")
return fmt.Errorf(`"root_dir" must be either empty or "/" and "auth.iam" must be true when "access_point_id" is used`)
}
return nil
}
Expand Down
16 changes: 14 additions & 2 deletions internal/pkg/manifest/validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ func TestAdvancedCount_Validate(t *testing.T) {
AdvancedCount: AdvancedCount{
Requests: aws.Int(123),
},
wantedError: fmt.Errorf(`"range" must be specified if "cpu_percentage", "memory_percentage", "requests", "response_time" are specified`),
wantedError: fmt.Errorf(`"range" must be specified if "cpu_percentage, memory_percentage, requests or response_time" are specified`),
},
}
for name, tc := range testCases {
Expand Down Expand Up @@ -297,6 +297,18 @@ func TestRangeConfig_Validate(t *testing.T) {

wantedError error
}{
"error if min is set but max is not": {
RangeConfig: RangeConfig{
Min: aws.Int(2),
},
wantedError: fmt.Errorf(`"max" must be specified if "min" is specified`),
},
"error if max is set but min is not": {
RangeConfig: RangeConfig{
Max: aws.Int(1),
},
wantedError: fmt.Errorf(`"min" must be specified if "max" is specified`),
},
"error if range min is greater than max": {
RangeConfig: RangeConfig{
Min: aws.Int(2),
Expand Down Expand Up @@ -420,7 +432,7 @@ func TestEFSVolumeConfiguration_Validate(t *testing.T) {
AccessPointID: aws.String("mockID"),
},
},
wantedError: fmt.Errorf("root_dir must be either empty or / and auth.iam must be true when access_point_id is in used"),
wantedError: fmt.Errorf(`"root_dir" must be either empty or "/" and "auth.iam" must be true when "access_point_id" is used`),
},
}
for name, tc := range testCases {
Expand Down

0 comments on commit 62c7c91

Please sign in to comment.