feat: enable multideployments #5324

bvtujo · 2023-09-25T17:51:16Z

Adds multideployments and an --all flag.

Customers can now specify a deployment order tag with their workload names, eg. "fe/1,be/2" to deploy services in a particular order.
Shared numbers create a deployment group, which can be parallelized in the future.

tk: unit tests for new behavior

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

Co-authored-by: Janice Huang <60631893+huanjani@users.noreply.github.com>

Lou1415926 · 2023-09-25T22:50:22Z

internal/pkg/cli/deploy.go

+				name: workload,
+				cmd:  deployCmd,
+			})
+			if err := deployCmd.Ask(); err != nil {


Am I correct that we would be prompting "which environment do you want to deploy xxx to" for each service xxx?

No, env gets asked once, separately, early on in Run(). Then, when loadWkldCmd gets called, it uses the env value provided earlier to populate that variable in each individual command. Ask() is really there to catch esoteric less-used flags that are workload specific.

internal/pkg/cli/deploy.go

Lou1415926 · 2023-09-25T22:55:12Z

internal/pkg/cli/deploy.go

+	count := 0
+	for i := 0; i < len(cmds); i++ {
+		count += len(cmds[i])


I guess we could also have count defined earlier, and increment it while we loop through each deploymentOrderGroup (the nested for-loop above), just to have one less for-loop iteration (not a huge difference though)

Lou1415926 · 2023-09-25T22:55:32Z

internal/pkg/cli/deploy.go

 	}
-	if err := o.deployWkld.RecommendActions(); err != nil {
-		return err
+	log.Infof("Will deploy %d %s in the following order.\n", count, english.PluralWord(count, "workload", ""))


Nice information here!

internal/pkg/cli/deploy.go

iamhopaul123 · 2023-09-27T21:22:59Z

internal/pkg/cli/deploy.go

 }

+func parseDeploymentOrderTags(namesWithOptionalOrder []string) (map[string]int, error) {
+	prioritiesMap := make(map[string]int)


Why do we use "priorities" instead of "orders" here?

this is a first step to parallelizing deployments--the idea is to create multiple groups which may be deployed in parallel, and then deploy those groups in a prescribed order. I wanted to use a different word to distinguish this, which is a precursor data structure, from the final.

internal/pkg/cli/deploy.go

github-actions · 2023-10-02T17:37:45Z

🍕 Here are the new binary sizes!

Name	New size (kiB)	size (kiB)	Delta (%)
macOS (amd)	51960	51740	+0.43
macOS (arm)	52816	52596	+0.42
linux (amd)	45700	45496	+0.45
linux (arm)	44996	44804	+0.43
windows (amd)	43156	42968	+0.44

codecov-commenter · 2023-10-02T17:41:49Z

Codecov Report

Attention: 60 lines in your changes are missing coverage. Please review.

Comparison is base (439abd8) 69.82% compared to head (efb00e9) 69.86%.

Additional details and impacted files

@@             Coverage Diff              @@
##           mainline    #5324      +/-   ##
============================================
+ Coverage     69.82%   69.86%   +0.03%     
============================================
  Files           296      297       +1     
  Lines         44835    45020     +185     
  Branches        287      287              
============================================
+ Hits          31307    31453     +146     
- Misses        12009    12038      +29     
- Partials       1519     1529      +10

Files	Coverage Δ
internal/pkg/cli/flag.go	`90.47% <ø> (ø)`
internal/pkg/queue/queue.go	`100.00% <100.00%> (ø)`
internal/pkg/term/selector/selector.go	`78.44% <100.00%> (-0.37%)`	⬇️
internal/pkg/cli/deploy.go	`55.45% <73.68%> (+9.52%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…s are uninitialized.

internal/pkg/cli/deploy.go

Lou1415926 · 2023-10-02T21:33:08Z

internal/pkg/cli/deploy.go

-	if o.yesInitWkld == nil {
-		confirmInitWkld, err := o.prompt.Confirm(fmt.Sprintf("Found manifest for uninitialized %s %q. Initialize it?", workloadType, name), "This workload will be initialized, then deployed.", prompt.WithFinalMessage(fmt.Sprintf("Initialize %s:", workloadType)))


Just curious, why do we remove this prompt? (I don't have an opinion on this yeet, just curious)

@dannyrandall made a great comment in bugbash feedback that simply specifying an un-initialized service is sufficient to divine the customer's intent to init it. This changes makes --init-wkld into a flag which specifically modifies the behavior of --all. That is, when --all and --init-wkld are true, copilot deploy will initialize and deploy everything in the ws. But when --init-wkld is false or unspecified, we'll only deploy initialized workloads.

Lou1415926 · 2023-10-02T22:22:00Z

internal/pkg/cli/deploy.go

+}
+
+// Pop removes the last element from the array.
+func (p *pq) Pop() any {


~~Not a request for this PR, but I feel like ideally we'd have a separate file / package for this and have some unit tests implemented on it!~~ Edit: oops, nvm, I think we don't need that at all. This is just to satisfy the interface that the heap package wants.

lmao too late I already wrote a generic priority queue

internal/pkg/cli/deploy.go

internal/pkg/term/selector/selector.go

internal/pkg/cli/deploy.go

Lou1415926

Just a few more comments!

internal/pkg/cli/deploy.go

Lou1415926 · 2023-10-03T16:48:53Z

internal/pkg/cli/deploy.go

+	}
+
+	for g, deploymentGroup := range cmds {
+		// TODO parallelize this. Steps involve:


Lou1415926 · 2023-10-03T21:42:01Z

internal/pkg/cli/deploy.go

+//	FilterItemsByStrings(wantedStrings, stringSlice2, func(s string) string { return s })
+//
+// It returns a list of strings (items whose stringFunc() exists in the list of wantedStrings).
+func FilterItemsByStrings[T any](wantedStrings []string, possibleItems []T, stringFunc func(T) string) []string {


Suggested change

func FilterItemsByStrings[T any](wantedStrings []string, possibleItems []T, stringFunc func(T) string) []string {

func filterItemsByStrings[T any](wantedStrings []string, possibleItems []T, stringFunc func(T) string) []string {

Lou1415926 · 2023-10-03T21:42:12Z

internal/pkg/cli/deploy.go

+//	func(w *config.Workload) string { return w.Name }
+//
+// as the stringFunc parameter.
+func FilterOutItems[T any](allItems []string, unwantedItems []T, stringFunc func(T) string) []string {


Suggested change

func FilterOutItems[T any](allItems []string, unwantedItems []T, stringFunc func(T) string) []string {

func filterOutItems[T any](allItems []string, unwantedItems []T, stringFunc func(T) string) []string {

Lou1415926 · 2023-10-03T21:51:22Z

internal/pkg/cli/deploy.go

 	cmd.Flags().BoolVar(&deployEnvironment, deployEnvFlag, false, deployEnvFlagDescription)
 	cmd.Flags().BoolVar(&initEnvironment, yesInitEnvFlag, false, yesInitEnvFlagDescription)
-	cmd.Flags().BoolVar(&initWorkload, yesInitWorkloadFlag, false, yesInitWorkloadFlagDescription)
+	cmd.Flags().BoolVar(&vars.yesInitWkld, yesInitWorkloadFlag, false, yesInitWorkloadFlagDescription)


Is the default true?

no, I think we should err on the side of only deploying initted local workloads unless the customer opts in.

iamhopaul123

🎉

Adds multideployments and an --all flag. Customers can now specify a deployment order tag with their workload names, eg. "fe/1,be/2" to deploy services in a particular order. Shared numbers create a deployment group, which can be parallelized in the future. tk: unit tests for new behavior By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the Apache 2.0 License.

bvtujo and others added 15 commits September 14, 2023 10:15

chore: add scaffolding to do multiple deployments in a loop

6bb3674

test: add unit test for loop behavior

dc8492c

enable multiservice flag

61655d3

chore: clean up no-ops

7606cd1

Merge branch 'mainline' into chore/multiple-svc-deployments

7db2a14

Update internal/pkg/cli/flag.go

be8b45c

Co-authored-by: Janice Huang <60631893+huanjani@users.noreply.github.com>

chore: remove all flag to satisfy linter and fix failing test

03dcd85

Merge branch 'mainline' into chore/multiple-svc-deployments

e923b97

chore: address feedback

ddb33a7

Merge branch 'mainline' into chore/multiple-svc-deployments

beb6ad5

fix: use established method to check wl type

8091859

chore: change flag to string slice

2e7f9e6

chore: add generic filter function

7658a12

feat: enable --all flag and deployment order tags

d020fce

docs: update deploy docstring

dc1abe5

bvtujo requested a review from a team as a code owner September 25, 2023 17:51

bvtujo requested review from efekarakus and removed request for a team September 25, 2023 17:51

chore: adjust flag description

c7ecb9b

Lou1415926 reviewed Sep 25, 2023

View reviewed changes

fix: several bugfixes

9bb2230

iamhopaul123 reviewed Sep 27, 2023

View reviewed changes

bvtujo added 3 commits September 28, 2023 14:13

fix: don't prompt for workload initialization

d9fee93

fix: don't error out on no infra changes

8f97bf5

chore: address feedback

44f82eb

iamhopaul123 reviewed Sep 29, 2023

View reviewed changes

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

bvtujo added 3 commits October 2, 2023 12:46

Merge branch 'mainline' into feat/enable-multiselect

e2033b0

chore: remove unsafe array index

64fd6d2

chore: embed actionCommand into struct

cf94dfb

bvtujo added 4 commits October 2, 2023 15:33

chore: use priority queue for deployment order; add unit tests

d9732fe

chore: add new test case for local workloads only

9183d7e

fix: update behavior for unspecified initWkld flag when some workload…

0e59042

…s are uninitialized.

nit: fix error in rendering deployment order

8217f47

Lou1415926 reviewed Oct 2, 2023

View reviewed changes

iamhopaul123 reviewed Oct 2, 2023

View reviewed changes

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

bvtujo added 2 commits October 3, 2023 10:59

fix: don't panic when there's an empty changeset

e4edaa9

chore: duplicate FilterItemsByStrings and FilterOutItems

be30e6f

Lou1415926 reviewed Oct 3, 2023

View reviewed changes

bvtujo added 7 commits October 3, 2023 13:16

chore: move pq to new package

53998c7

chore: fix unit test

bc01562

chore: staticcheck

17fcd6d

chore: example to Lesser struct

f4e66b0

chore: address nits

8beec42

chore: fix staticcheck (unused variable)

5a53631

chore: make initwkld a bool, not *bool, and update description

44e97b8

Lou1415926 approved these changes Oct 3, 2023

View reviewed changes

bvtujo added 2 commits October 4, 2023 10:15

chore: don't export funcs; rewrite docstrings

eafbce3

chore: nits

a01ad23

iamhopaul123 approved these changes Oct 4, 2023

View reviewed changes

Merge branch 'mainline' into feat/enable-multiselect

efb00e9

mergify bot merged commit cbb1718 into mainline Oct 4, 2023

mergify bot deleted the feat/enable-multiselect branch October 4, 2023 16:35

		if o.yesInitWkld == nil {
		confirmInitWkld, err := o.prompt.Confirm(fmt.Sprintf("Found manifest for uninitialized %s %q. Initialize it?", workloadType, name), "This workload will be initialized, then deployed.", prompt.WithFinalMessage(fmt.Sprintf("Initialize %s:", workloadType)))

	func FilterItemsByStrings[T any](wantedStrings []string, possibleItems []T, stringFunc func(T) string) []string {
	func filterItemsByStrings[T any](wantedStrings []string, possibleItems []T, stringFunc func(T) string) []string {

	func FilterOutItems[T any](allItems []string, unwantedItems []T, stringFunc func(T) string) []string {
	func filterOutItems[T any](allItems []string, unwantedItems []T, stringFunc func(T) string) []string {

feat: enable multideployments #5324

feat: enable multideployments #5324

Uh oh!

Conversation

bvtujo commented Sep 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lou1415926 Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lou1415926 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamhopaul123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions bot commented Oct 2, 2023 •

edited

Loading

codecov-commenter commented Oct 2, 2023 •

edited

Loading

Lou1415926 Oct 2, 2023 •

edited

Loading