chore: Adds DependsOn converters and logic #2421

xar-tol · 2021-06-04T17:10:39Z

Adds dependency logic and conversion functions.

Addresses logic/conversions for #2175 .

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

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

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

huanjani

This looks really great! Very thorough unit tests!

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

clarifies convertDependsOn adjusts errors to be lowercase

iamhopaul123

Wow this looks amazing!

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

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

iamhopaul123 · 2021-06-07T17:44:11Z

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

+
+var (
+	// Container dependency status options
+	DependsOnStatus = []string{"start", "complete", "success"}


Can we make "start"... constants? Also how about "healthy"? #2419 should be able to unblock it.

Is container health check in scheduled job? I believe the blocker was both the LB web service and the scheduled job.

good point. i'll adjust the PR to add schedule job as well. maybe ignore this comment until they are all fixed

Since you added it to your PR, I added healthy.

Does this mean that we need to add some extra validation to stop main containers from depending on sidecar: healthy conditions? Since sidecars don't have container health checks there's no way to determine sidecar health.

Adjusted methods for finding essential containers and valid status to consider that sidecars can't have healthy condition.

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

adjusts graph to custom version adds datastructure for convert sidecar adds convertImageDependsOn to jobs Adjusts essential check to use aws.bool Removes unecessary circular dependency check Makes invalid container errors return themselves and not nil

adjusts functions to use convertSidecarOpts instead of individual parameters

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

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

bvtujo

Alexandra. This is so good. I just have some small comments and questions about the logic

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

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

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

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

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

adjusts errors to state dependencies for circular dependencies, adds error for dependsny on itself adjusts comments adjusts status to allow healthy and adds function to ensure uppercase better modularizes cyclic dependency detection better modularizes essential status Please enter the commit message for your changes. Lines starting

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

Lines starting

bvtujo · 2021-06-09T19:59:59Z

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

@@ -120,49 +121,88 @@ func validateSidecarDependsOn(in manifest.SidecarConfig, sidecarName string, s c
 	}

 	for name, status := range in.DependsOn {
-		// status must be one of < start | complete | success >
 		if !isValidStatus(status) {


Can we add a conversion to lowercase here, so that Healthy in the manifest can get converted here to healthy, register as valid, then get coverted to HEALTHY in the CFN template?

All statuses are converted to uppercase before it gets into validate through the transformations, (sidecar use) (image use) so I don't think it's necessary, unless things can bypass that check?

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

bvtujo · 2021-06-09T21:05:59Z

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

 }

-func (graph *Graph) Add(fromNode, toNode string) {
+func (g *graph) Add(fromNode, toNode string) {


Can we make this function private too, since we aren't reusing this struct elsewhere?

Good point! Adjusted.

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

adjusts essential status check and valid status check to consider that sidecars can never be "HEALTHY" adds errors for the above for sidecars adjusts errors to be less deeply nested for circular dependencies makes graph function private refactors acyclic to return early

iamhopaul123

ship it feel free to remove the label whenever the PR is ready

bvtujo

Extremely nice refactor. This is such a cool little piece of code! I'm approving this because my comments are just naming nits and small readability refactors and don't reflect any major changes. Feel free to remove the do-not-merge label when you've addressed them!

bvtujo · 2021-06-10T20:28:20Z

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

 		}
-		if isEssential(name, s) && !isEssentialStatus(status) {
-			return errEssentialContainerStatus
+		if ok, err := isEssentialStatus(status, name, s); isEssential(name, s) && !ok {


Can we add a quick comment here explaining this bit of logic?

// Container cannot depend on essential containers having status `complete` or `success`

Added a clarifying comment.

bvtujo · 2021-06-10T20:40:48Z

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

-	for _, allowed := range dependsOnStatus {
-		if s == allowed {
-			return true
+func isValidStatus(status string, container string, c convertSidecarOpts) (bool, error) {


Can we rename this to isValidStatusForContainer? It might be a bit more transparent what we're checking that way.

Adjusted naming.

bvtujo · 2021-06-10T20:41:47Z

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

-			return true
+func isValidStatus(status string, container string, c convertSidecarOpts) (bool, error) {
+	if _, ok := c.sidecarConfig[container]; ok {
+		for _, allowed := range sidecarDependsOnStatus {


can we rename this const to sidecarDependsOnValidStatuses or sth similar for clarity?

bvtujo · 2021-06-10T20:49:03Z

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

-		if s == allowed {
-			return true
+func isEssentialStatus(status string, container string, c convertSidecarOpts) (bool, error) {
+	if _, ok := c.sidecarConfig[container]; ok {


since this bit of code is used pretty often, we might want to refactor it into a function:

func isSidecar(container string, sidecarConfig map[string]*manifest.SidecarConfig) bool

and this line can be replaced by:

Suggested change

if _, ok := c.sidecarConfig[container]; ok {

if isSidecar(container, c.sidecarConfig) {

Better modularized.

adjusts sidecar check to be its own function adjusts naming

Adds dependency logic and conversion functions. Addresses logic/conversions for aws#2175 . By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

xar-tol added 2 commits June 4, 2021 13:05

adjusts sidecar parameters due to adjusted conversion logic

cc58eae

adds dependency validation logic

c3e3f25

xar-tol self-assigned this Jun 4, 2021

xar-tol requested a review from a team as a code owner June 4, 2021 17:10

xar-tol requested a review from huanjani June 4, 2021 17:10

xar-tol changed the title ~~Xar tol/sidecar dependency converters~~ Chore: Adds DependsOn converters and logic Jun 4, 2021

simplifies booleans

a6567ce

xar-tol changed the title ~~Chore: Adds DependsOn converters and logic~~ chore: Adds DependsOn converters and logic Jun 4, 2021

huanjani reviewed Jun 4, 2021

View reviewed changes

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

huanjani reviewed Jun 4, 2021

View reviewed changes

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

huanjani approved these changes Jun 4, 2021

View reviewed changes

huanjani reviewed Jun 4, 2021

View reviewed changes

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

adjusts formatting

ed913b2

clarifies convertDependsOn adjusts errors to be lowercase

bvtujo added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 4, 2021

iamhopaul123 reviewed Jun 7, 2021

View reviewed changes

adjusts graph, addreses comments

f20095d

adjusts graph to custom version adds datastructure for convert sidecar adds convertImageDependsOn to jobs Adjusts essential check to use aws.bool Removes unecessary circular dependency check Makes invalid container errors return themselves and not nil

xar-tol requested a review from iamhopaul123 June 7, 2021 20:05

iamhopaul123 assigned iamhopaul123 and unassigned xar-tol Jun 7, 2021

adjusts functions for use of convertSidecarOpts

2850882

adjusts functions to use convertSidecarOpts instead of individual parameters

iamhopaul123 reviewed Jun 7, 2021

View reviewed changes

bvtujo reviewed Jun 7, 2021

View reviewed changes

xar-tol requested review from iamhopaul123 and bvtujo June 8, 2021 19:44

adjusts essential container status error

b1245b6

iamhopaul123 reviewed Jun 9, 2021

View reviewed changes

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

removes uncessary check for dependsOn size

6966c94

Lines starting

bvtujo reviewed Jun 9, 2021

View reviewed changes

xar-tol added 2 commits June 10, 2021 11:26

adjusts nil format to comma ok format

07487db

xar-tol requested review from iamhopaul123 and bvtujo June 10, 2021 15:53

iamhopaul123 approved these changes Jun 10, 2021

View reviewed changes

bvtujo approved these changes Jun 10, 2021

View reviewed changes

xar-tol added 2 commits June 10, 2021 17:25

adjusts modularity of sidecar check, naming

59d909f

adjusts sidecar check to be its own function adjusts naming

adjusts status variable naming to statuses

93f6fc6

xar-tol removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Jun 10, 2021

Merge branch 'mainline' into xar-tol/sidecar-dependency-converters

258d91e

mergify bot merged commit bd2c70f into aws:mainline Jun 10, 2021

xar-tol deleted the xar-tol/sidecar-dependency-converters branch August 5, 2021 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Adds DependsOn converters and logic #2421

chore: Adds DependsOn converters and logic #2421

xar-tol commented Jun 4, 2021

huanjani left a comment

iamhopaul123 left a comment

iamhopaul123 Jun 7, 2021

xar-tol Jun 7, 2021

iamhopaul123 Jun 7, 2021

xar-tol Jun 8, 2021

bvtujo Jun 9, 2021

xar-tol Jun 10, 2021

bvtujo left a comment

bvtujo Jun 9, 2021

xar-tol Jun 10, 2021 •

edited

Loading

bvtujo Jun 9, 2021

xar-tol Jun 10, 2021

iamhopaul123 left a comment •

edited

Loading

bvtujo left a comment

bvtujo Jun 10, 2021

xar-tol Jun 10, 2021

bvtujo Jun 10, 2021

xar-tol Jun 10, 2021

bvtujo Jun 10, 2021

xar-tol Jun 10, 2021

bvtujo Jun 10, 2021

xar-tol Jun 10, 2021

	if _, ok := c.sidecarConfig[container]; ok {
	if isSidecar(container, c.sidecarConfig) {

chore: Adds DependsOn converters and logic #2421

chore: Adds DependsOn converters and logic #2421

Conversation

xar-tol commented Jun 4, 2021

huanjani left a comment

Choose a reason for hiding this comment

iamhopaul123 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

bvtujo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xar-tol Jun 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamhopaul123 left a comment • edited Loading

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

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

xar-tol Jun 10, 2021 •

edited

Loading

iamhopaul123 left a comment •

edited

Loading