chore(manifest): add from_tag for workload #3727

iamhopaul123 · 2022-07-06T23:52:21Z

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

internal/pkg/deploy/cloudformation/stack/transformers.go

internal/pkg/cli/deploy/svc.go

internal/pkg/manifest/workload_test.go

internal/pkg/manifest/workload.go

internal/pkg/cli/deploy/svc.go

internal/pkg/manifest/lb_web_svc.go

efekarakus

Posting here as well for increase visibility. We were discussing offline how we can make adding the from_tags option to other fields easier and here is our code specification proposal:

https://gist.github.com/efekarakus/5859927876e87528f14117cfbd779833

efekarakus · 2022-07-07T22:56:24Z

internal/pkg/aws/ec2/ec2.go

@@ -50,6 +50,17 @@ type Filter struct {
 	Values []string
 }

+// FilterForTags takes a key and optional values to construct an EC2 filter.
+func FilterForTags(key string, values ...string) Filter {


Nice, this is a really clean function signature 👍 😍

bvtujo

nice work boss, looks good to me

bvtujo · 2022-07-14T16:15:09Z

internal/pkg/cli/job_deploy.go

@@ -44,6 +45,7 @@ type deployJobOpts struct {
 	// cached variables
 	targetApp       *config.Application
 	targetEnv       *config.Environment
+	envSess         *session.Session


😰 were we not using the correct session for job deploy this entire time>>>

it is actually used here

bvtujo · 2022-07-14T18:57:20Z

internal/pkg/manifest/loader.go

+}
+
+// DynamicWorkloadManifest represents a dynamically populated workload manifest.
+type DynamicWorkloadManifest struct {


this makes so much sense as the next evolution of the manifest types

internal/pkg/aws/ec2/ec2.go

internal/pkg/cli/svc_deploy.go

internal/pkg/manifest/loader.go

internal/pkg/manifest/workload.go

Lou1415926

Late to the party but 😂 adding the comments just for your reference

Lou1415926 · 2022-07-07T23:47:17Z

internal/pkg/manifest/workload.go

-		case *yaml.TypeError:
-			break
-		default:
+		var yamlTypeErr *yaml.TypeError


Lou1415926 · 2022-07-22T16:48:01Z

internal/pkg/aws/ec2/ec2.go

@@ -333,7 +344,9 @@ func (c *EC2) subnets(filters ...Filter) ([]*ec2.Subnet, error) {
 		}
 		subnets = append(subnets, response.Subnets...)
 	}
-
+	if len(subnets) == 0 {
+		return nil, fmt.Errorf("cannot find any subnets")


Suggested change

return nil, fmt.Errorf("cannot find any subnets")

return nil, errors.New("cannot find any subnets")

Lou1415926 · 2022-07-22T16:57:10Z

internal/pkg/deploy/cloudformation/stack/lb_grpc_web_service_integration_test.go

 		err = envMft.Validate()
 		require.NoError(t, err)
+		content := envMft.Manifest()


Do you thing we also want to add the .Load step to these local integration test? I feel like otherwise they wouldn't be testing the right thing because in real executions their output template would be one that has gone through .Load.

If we add it it is not local anymore lol

It is not local only if it has subnet.from_tags and not subnet.ids right?

Load is supposed to load dynamic contents that requires API calls. If the test itself is supposed to be run locally then it doesn't make sense to me to add .Load step 🤔

I feel like the only reason why these tests have all these Validate, ApplyEnv calls is because we didn't have a better way to mimic how cli package would have manipulate the manifest, than to just hardcoding-ly repeating the codes in cli. Without calling Load, we are not effectively "mimicking" it.

Another way that I think about it is that I think these integration tests were meant to test that "given a manifest input, do I eventually get an expected template". Without calling Load like cli does, we can't be confident with our test result.

that's true. IMO we need to

remove some of our existing integ tests because we have e2e tests now

make current local integ tests to be integ tests because we can't generate expected template without making internet.

Lou1415926 · 2022-07-22T17:03:44Z

internal/pkg/manifest/transform.go

@@ -20,10 +20,11 @@ var defaultTransformers = []mergo.Transformers{
 	imageTransformer{},
 	buildArgsOrStringTransformer{},
 	aliasTransformer{},
-	stringSliceOrStringTransformer{},
+	StringSliceOrStringTransformer{},


Does this transformer need to be exported?

Lou1415926 · 2022-07-22T17:06:00Z

internal/pkg/task/env_runner.go

@@ -23,8 +23,8 @@ const (

 // Names for tag filters
 var (
-	tagFilterNameForApp = fmt.Sprintf(ec2.TagFilterName, deploy.AppTagKey)
-	tagFilterNameForEnv = fmt.Sprintf(ec2.TagFilterName, deploy.EnvTagKey)
+	FmtTagFilterForApp = fmt.Sprintf(ec2.FmtTagFilter, deploy.AppTagKey)


I might be missing something - do they need to be exported?

Lou1415926 · 2022-07-22T17:15:34Z

internal/pkg/manifest/workload.go

+// UnmarshalYAML overrides the default YAML unmarshaling logic for the StringSliceOrString
+// struct, allowing it to perform more complex unmarshaling behavior.
+// This method implements the yaml.Unmarshaler (v3) interface.
+func (s *StringSliceOrString) UnmarshalYAML(value *yaml.Node) error {


This is essentially the same method as unmarshalYAMLToStringSliceOrString(s *StringSliceOrString, value *yaml.Node) error. Maybe we can keep this one and remove that one. Then we can replace all

unmarshalYAMLToStringSliceOrString((*stringSliceOrString)(e), value)

to

(*stringSliceOrString)(e).UnmarshalYAML(value)

Lou1415926 · 2022-07-22T17:32:21Z

internal/pkg/manifest/workload.go

+		values := v.StringSlice
+		if v.String != nil {
+			values = []string{aws.StringValue(v.String)}
+		}


Does

values := v.toStringSlice()

do what you want here?

Lou1415926 · 2022-07-22T17:39:32Z

internal/pkg/manifest/workload.go

+}
+
+// Load populates the subnet's IDs field if the client is using tags.
+func (dyn *dynamicSubnets) load() error {


I see this method is tested in the DynamicWorkload's Load's test. I think it makes sense. In the future, when we have more and more loader though, we probably would want to test each individual loader separately 💭

Lou1415926 · 2022-07-22T17:51:48Z

internal/pkg/manifest/workload.go

+}
+
+type workloadManifest interface {
+	Validate() error


Could this be an unexported function as well?

Lou1415926 · 2022-07-22T18:03:09Z

internal/pkg/deploy/cloudformation/stack/transformers.go

-	}
-	opts.SubnetsType = subnetPlacementForTemplate[*placement.PlacementString]
+	opts.AssignPublicIP = template.DisablePublicIP
+	opts.SubnetsType = ""


tiny nit: is this line here just to be sure we aren't setting SubnetsType? we could probably remove it

this is actually to unset the subnet types since it is contradictory to subnetIDs.

oh lol didn't realize it was set to template.PublicSubnetsPlacement by default. I feel like this function probably also needs some refactoring in the future 😂

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

Also addresses fb from #3727 (review) By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

iamhopaul123 requested a review from a team as a code owner July 6, 2022 23:52

iamhopaul123 requested review from dannyrandall and removed request for a team July 6, 2022 23:52

amazon-ecs-cli-v2-pr-manager requested a review from Lou1415926 July 6, 2022 23:52

paragbhingre reviewed Jul 7, 2022

View reviewed changes

internal/pkg/deploy/cloudformation/stack/transformers.go Outdated Show resolved Hide resolved

paragbhingre reviewed Jul 7, 2022

View reviewed changes

internal/pkg/deploy/cloudformation/stack/transformers.go Outdated Show resolved Hide resolved

paragbhingre reviewed Jul 7, 2022

View reviewed changes

internal/pkg/cli/deploy/svc.go Outdated Show resolved Hide resolved

paragbhingre reviewed Jul 7, 2022

View reviewed changes

internal/pkg/manifest/workload_test.go Show resolved Hide resolved

dannyrandall reviewed Jul 7, 2022

View reviewed changes

internal/pkg/manifest/workload.go Outdated Show resolved Hide resolved

internal/pkg/cli/deploy/svc.go Outdated Show resolved Hide resolved

internal/pkg/manifest/lb_web_svc.go Outdated Show resolved Hide resolved

iamhopaul123 force-pushed the subnets-from-tags branch from 8d63001 to c04895a Compare July 7, 2022 19:03

efekarakus reviewed Jul 8, 2022

View reviewed changes

iamhopaul123 marked this pull request as draft July 8, 2022 20:04

iamhopaul123 force-pushed the subnets-from-tags branch from c04895a to 36a67d6 Compare July 13, 2022 23:08

iamhopaul123 marked this pull request as ready for review July 13, 2022 23:08

bvtujo approved these changes Jul 14, 2022

View reviewed changes

dannyrandall reviewed Jul 14, 2022

View reviewed changes

internal/pkg/aws/ec2/ec2.go Show resolved Hide resolved

internal/pkg/cli/svc_deploy.go Outdated Show resolved Hide resolved

internal/pkg/manifest/loader.go Outdated Show resolved Hide resolved

dannyrandall reviewed Jul 14, 2022

View reviewed changes

internal/pkg/manifest/workload.go Show resolved Hide resolved

efekarakus added do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. and removed do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. labels Jul 18, 2022

chore(manifest): add DynamicWorkloadManifest

7f48afd

Lou1415926 added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jul 22, 2022

Addr dnrnd fb

e0b4f40

iamhopaul123 force-pushed the subnets-from-tags branch from 36a67d6 to e0b4f40 Compare July 22, 2022 17:11

iamhopaul123 added 2 commits July 22, 2022 10:28

Change to any, fix flaky unit test and regenerate mocks

e0fffdb

Remove outdated mock file

92eb9b7

iamhopaul123 removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jul 22, 2022

dannyrandall approved these changes Jul 22, 2022

View reviewed changes

mergify bot merged commit 4595a61 into aws:mainline Jul 22, 2022

Lou1415926 reviewed Jul 22, 2022

View reviewed changes

iamhopaul123 mentioned this pull request Jul 22, 2022

chore: fix broken deploy #3801

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(manifest): add from_tag for workload #3727

chore(manifest): add from_tag for workload #3727

iamhopaul123 commented Jul 6, 2022

efekarakus left a comment •

edited

Loading

efekarakus Jul 7, 2022

bvtujo left a comment

bvtujo Jul 14, 2022

iamhopaul123 Jul 22, 2022

bvtujo Jul 14, 2022

Lou1415926 left a comment

Lou1415926 Jul 7, 2022

Lou1415926 Jul 22, 2022

Lou1415926 Jul 22, 2022

iamhopaul123 Jul 22, 2022

Lou1415926 Jul 22, 2022

iamhopaul123 Jul 22, 2022

Lou1415926 Jul 22, 2022

iamhopaul123 Jul 22, 2022

Lou1415926 Jul 22, 2022

Lou1415926 Jul 22, 2022

Lou1415926 Jul 22, 2022

Lou1415926 Jul 22, 2022

Lou1415926 Jul 22, 2022

Lou1415926 Jul 22, 2022 •

edited

Loading

Lou1415926 Jul 22, 2022

iamhopaul123 Jul 22, 2022

Lou1415926 Jul 22, 2022

	return nil, fmt.Errorf("cannot find any subnets")
	return nil, errors.New("cannot find any subnets")

chore(manifest): add from_tag for workload #3727

chore(manifest): add from_tag for workload #3727

Conversation

iamhopaul123 commented Jul 6, 2022

efekarakus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bvtujo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lou1415926 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lou1415926 Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efekarakus left a comment •

edited

Loading

Lou1415926 Jul 22, 2022 •

edited

Loading