Skip to content
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.

Sign up for GitHub

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

chore(manifest): add taskdef_override and parsing func #2743

Merged
merged 3 commits into from
Aug 17, 2021
Merged

chore(manifest): add taskdef_override and parsing func #2743

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5cc5f6b
chore(manifest): add taskdef_override and parsing func
iamhopaul123 Aug 16, 2021
f948a7c
Address feedback
iamhopaul123 Aug 17, 2021
ed142e9
Merge branch 'mainline' into taskdef-override-manifest
mergify[bot] Aug 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions internal/pkg/manifest/backend_svc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,14 @@ type BackendService struct {

// BackendServiceConfig holds the configuration that can be overridden per environments.
type BackendServiceConfig struct {
ImageConfig ImageWithPortAndHealthcheck `yaml:"image,flow"`
ImageOverride `yaml:",inline"`
TaskConfig `yaml:",inline"`
*Logging `yaml:"logging,flow"`
Sidecars map[string]*SidecarConfig `yaml:"sidecars"`
Network *NetworkConfig `yaml:"network"`
Publish *PublishConfig `yaml:"publish"`
OverrideRule `yaml:",inline"`
ImageConfig ImageWithPortAndHealthcheck `yaml:"image,flow"`
ImageOverride `yaml:",inline"`
TaskConfig `yaml:",inline"`
*Logging `yaml:"logging,flow"`
Sidecars map[string]*SidecarConfig `yaml:"sidecars"`
Network *NetworkConfig `yaml:"network"`
Publish *PublishConfig `yaml:"publish"`
TaskDefOverrides []OverrideRule `yaml:"taskdef_overrides"`
}

// NewBackendService applies the props to a default backend service configuration with
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg/manifest/job.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ type ScheduledJobConfig struct {
JobFailureHandlerConfig `yaml:",inline"`
Network *NetworkConfig `yaml:"network"`
Publish *PublishConfig `yaml:"publish"`
OverrideRule `yaml:",inline"`
TaskDefOverrides []OverrideRule `yaml:"taskdef_overrides"`
}

// JobTriggerConfig represents the configuration for the event that triggers the job.
Expand Down
18 changes: 9 additions & 9 deletions internal/pkg/manifest/lb_web_svc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ type LoadBalancedWebService struct {

// LoadBalancedWebServiceConfig holds the configuration for a load balanced web service.
type LoadBalancedWebServiceConfig struct {
ImageConfig ImageWithPortAndHealthcheck `yaml:"image,flow"`
ImageOverride `yaml:",inline"`
RoutingRule `yaml:"http,flow"`
TaskConfig `yaml:",inline"`
*Logging `yaml:"logging,flow"`
Sidecars map[string]*SidecarConfig `yaml:"sidecars"`
Network *NetworkConfig `yaml:"network"` // TODO: the type needs to be updated after we upgrade mergo
Publish *PublishConfig `yaml:"publish"`
OverrideRule `yaml:",inline"`
ImageConfig ImageWithPortAndHealthcheck `yaml:"image,flow"`
ImageOverride `yaml:",inline"`
RoutingRule `yaml:"http,flow"`
TaskConfig `yaml:",inline"`
*Logging `yaml:"logging,flow"`
Sidecars map[string]*SidecarConfig `yaml:"sidecars"`
Network *NetworkConfig `yaml:"network"` // TODO: the type needs to be updated after we upgrade mergo
Publish *PublishConfig `yaml:"publish"`
TaskDefOverrides []OverrideRule `yaml:"taskdef_overrides"`
}

// RoutingRule holds the path to route requests to the service.
Expand Down
19 changes: 14 additions & 5 deletions internal/pkg/manifest/svc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ func TestUnmarshalSvc(t *testing.T) {
version: 1.0
name: frontend
type: "Load Balanced Web Service"
taskdef_overrides:
- path: "ContainerDefinitions[0].Ulimits[-].HardLimit"
value: !Ref ParamName
image:
location: foo/bar
credentials: some arn
Expand Down Expand Up @@ -57,8 +60,6 @@ logging:
secretOptions:
LOG_TOKEN: LOG_TOKEN
configFilePath: /extra.conf
taskdef_override:
- "ContainerDefinitions[0].Ulimits[-].HardLimit: !Ref ParamName"
environments:
test:
count: 3
Expand Down Expand Up @@ -143,9 +144,17 @@ environments:
Placement: stringP("public"),
},
},
OverrideRule: OverrideRule{
TaskDefOverrideRules: []TaskDefinitionOverrideRule{
"ContainerDefinitions[0].Ulimits[-].HardLimit: !Ref ParamName",
TaskDefOverrides: []OverrideRule{
{
Path: "ContainerDefinitions[0].Ulimits[-].HardLimit",
Value: yaml.Node{
Kind: 8,
Style: 1,
Tag: "!Ref",
Value: "ParamName",
Line: 7,
Column: 12,
},
},
},
},
Expand Down
16 changes: 8 additions & 8 deletions internal/pkg/manifest/worker_svc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,14 @@ type WorkerService struct {

// WorkerServiceConfig holds the configuration that can be overridden per environments.
type WorkerServiceConfig struct {
ImageConfig ImageWithHealthcheck `yaml:"image,flow"`
ImageOverride `yaml:",inline"`
TaskConfig `yaml:",inline"`
*Logging `yaml:"logging,flow"`
Sidecars map[string]*SidecarConfig `yaml:"sidecars"`
Subscribe *SubscribeConfig `yaml:"subscribe"`
Network *NetworkConfig `yaml:"network"`
OverrideRule `yaml:",inline"`
ImageConfig ImageWithHealthcheck `yaml:"image,flow"`
ImageOverride `yaml:",inline"`
TaskConfig `yaml:",inline"`
*Logging `yaml:"logging,flow"`
Sidecars map[string]*SidecarConfig `yaml:"sidecars"`
Subscribe *SubscribeConfig `yaml:"subscribe"`
Network *NetworkConfig `yaml:"network"`
TaskDefOverrides []OverrideRule `yaml:"taskdef_overrides"`
}

// WorkerServiceProps represents the configuration needed to create a worker service.
Expand Down
25 changes: 2 additions & 23 deletions internal/pkg/manifest/workload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,9 @@ import (
"fmt"
"path/filepath"
"strconv"
"strings"
"time"

"github.com/aws/copilot-cli/internal/pkg/docker/dockerengine"
"github.com/aws/copilot-cli/internal/pkg/template/override"

"github.com/dustin/go-humanize/english"

Expand Down Expand Up @@ -78,27 +76,8 @@ type Workload struct {

// OverrideRule holds the manifest overriding rule for CloudFormation template.
type OverrideRule struct {
TaskDefOverrideRules []TaskDefinitionOverrideRule `yaml:"taskdef_override"`
}

// TaskDefinitionOverrideRule holds the overriding rule for ECS Task Definition.
type TaskDefinitionOverrideRule string

// Parse parses the manifest ECS Task Definition overriding rule into override.Rule.
func (r TaskDefinitionOverrideRule) Parse() (*override.Rule, error) {
// Make sure the overriding rule is in a map format.
// For example: "ContainerDefinitions[0].Ulimits[-].HardLimit: !Ref ParamName"
if len(strings.Split(string(r), ":")) != 2 {
return nil, fmt.Errorf("invalid overriding rule %s", r)
}
var node yaml.Node
if err := yaml.Unmarshal([]byte(r), &node); err != nil {
return nil, fmt.Errorf("unmarshal task definition overriding rule %s: %w", r, err)
}
return &override.Rule{
Path: node.Content[0].Content[0].Value,
Value: node.Content[0].Content[1],
}, nil
Path string `yaml:"path"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

look at all that sweet deleted code 😎

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need Path *string and Value *yaml.Node?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Value *yaml.Node won't work...and I don't think empty string would be a valid value for Path

Copy link
Contributor

@Lou1415926 Lou1415926 Aug 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If path isn't specified in manifest, wont it get initialized as ""? In that case, how do we tell it from an actual invalid input where the user specifically put "" for path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/aws/copilot-cli/pull/2743/files#diff-05339d6eda8f6845d17d754ca2a2cb1b58ebd84ad763d1f9b31a961188eb2a4dR48 if it isn't specified then taskdef_overrides length would be 0 and we wouldn't even loop through it. Unless they have

taskdef_override:
  - Value: foo

then we'll error out.

Value yaml.Node `yaml:"value"`
}

// Image represents the workload's container image.
Expand Down
47 changes: 0 additions & 47 deletions internal/pkg/manifest/workload_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,61 +5,14 @@ package manifest

import (
"errors"
"fmt"
"path/filepath"
"testing"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/copilot-cli/internal/pkg/template/override"
"github.com/stretchr/testify/require"
"gopkg.in/yaml.v3"
)

func TestTaskDefinitionOverrideRule_Parse(t *testing.T) {
testCases := map[string]struct {
inOverrideRule string

wantedRule *override.Rule
wantedError error
}{
"error if invalid override rule": {
inOverrideRule: "badRule",
wantedError: fmt.Errorf("invalid overriding rule badRule"),
},
"error if fail to unmarshal override rule": {
inOverrideRule: ": foo",
wantedError: fmt.Errorf("unmarshal task definition overriding rule : foo: yaml: did not find expected key"),
},
"success": {
inOverrideRule: "ContainerDefinitions[0].Ulimits[-].HardLimit: !Ref ParamName",
wantedRule: &override.Rule{
Path: "ContainerDefinitions[0].Ulimits[-].HardLimit",
Value: &yaml.Node{
Kind: 8,
Style: 1,
Tag: "!Ref",
Value: "ParamName",
Line: 1,
Column: 47,
},
},
},
}

for name, tc := range testCases {
t.Run(name, func(t *testing.T) {
r := TaskDefinitionOverrideRule(tc.inOverrideRule)
got, err := r.Parse()
if tc.wantedError != nil {
require.EqualError(t, err, tc.wantedError.Error())
} else {
require.NoError(t, err)
require.Equal(t, tc.wantedRule, got)
}
})
}
}

func TestEntryPointOverride_UnmarshalYAML(t *testing.T) {
testCases := map[string]struct {
inContent []byte
Expand Down