feat: allow initializing workloads during copilot deploy by bvtujo · Pull Request #5168 · aws/copilot-cli

bvtujo · 2023-08-08T18:40:00Z

This is stacked on top of a couple of PRs (like 5128 and 5139). The diff should get smaller over time as those are merged.

Adds a new method to initialize which skips manifest writing.

I think the ideal scenario here is actually to use an initializer option on the workload initializer struct, but I wanted to get this out quickly rather than doing another big 300-line refactor if possible. Happy to do this after release; i actually have started the work.

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: Penghao He <penghaoh@amazon.com>

huanjani · 2023-08-11T18:49:51Z

+
+	// If init-wkld and no-init-wkld flags aren't set, prompt customer to initialize.
+	if !o.yesInitWkld {
+		o.yesInitWkld, err = o.prompt.Confirm(fmt.Sprintf("Found manifest for uninitialized %s %q. Initialize it?", workloadType, o.name), "This workload will be initialized, then deployed.", prompt.WithConfirmFinalMessage())


huanjani · 2023-08-11T18:54:01Z

+	yesInitWorkloadFlag = "init-wkld"
+	noInitWorkloadFlag  = "no-init-wkld"


Just a thought. Should this be a bool instead? I understand wanting consistency with the other flags (deploy/no-deploy). 🤷🏼

You don't actually get the right amount of granularity if it's just a bool.

For example, if we just use init-wkld and the flag is un-specified, it's false. Then do you prompt for initialization, or do you assume the user's intent is not to init the workload and error out? It leads to problems where there's no way to force the command to exit when the intent is not to init the workload if it doesn't exist, or there's no way to display a prompt when the intent is ambiguous. Perhaps a pointer to a bool would do it, but I don't think there's a way of specifying a bool flag like --init=false in cobra.

Long winded way of saying that as far as I can tell, the best way to specify choices like this where we want to prompt by default but allow customers to specify either a positive or a negative is to use two mutually exclusive bool flags.

It sounds to be very similar to

copilot-cli/internal/pkg/cli/svc_exec.go

Line 291 in 77c88c2

if cmd.Flags().Changed(yesFlag) {

🤔

CaptainCarpensir

Looks cool 👀
Couple nits

github-actions · 2023-08-18T15:40:00Z

🍕 Here are the new binary sizes!

Name	New size (kiB)	size (kiB)	Delta (%)
macOS (amd)	51632	51488	+0.28
macOS (arm)	51844	51684	+0.31
linux (amd)	45452	45324	+0.28
linux (arm)	43716	43588	+0.29
windows (amd)	42260	42144	+0.28

codecov-commenter · 2023-08-18T15:44:18Z

Codecov Report

Merging #5168 (9b35f0b) into mainline (86efee6) will decrease coverage by 0.01%.
Report is 1 commits behind head on mainline.
The diff coverage is 57.14%.

@@             Coverage Diff              @@
##           mainline    #5168      +/-   ##
============================================
- Coverage     69.52%   69.51%   -0.01%     
============================================
  Files           295      295              
  Lines         43897    43988      +91     
  Branches        285      285              
============================================
+ Hits          30518    30579      +61     
- Misses        11889    11913      +24     
- Partials       1490     1496       +6

Files Changed	Coverage Δ
internal/pkg/cli/deploy.go	`36.08% <49.23%> (+8.74%)`	⬆️
internal/pkg/cli/init.go	`25.59% <52.63%> (+1.75%)`	⬆️
internal/pkg/initialize/workload.go	`78.24% <100.00%> (+1.22%)`	⬆️

... and 4 files with indirect coverage changes

dannyrandall

i'm curious what the difference is between svc init --deploy and svc deploy after these changes?

dannyrandall · 2023-08-21T14:03:47Z

+			if cmd.Flags().Changed(yesInitWorkloadFlag) {
+				opts.yesInitWkld = aws.Bool(false)
+				if initWorkload {
+					opts.yesInitWkld = aws.Bool(true)
+				}
+			}


why this? instead of just setting vars.yesInitWkld down below??

ah i think sort of answered this below, but +1 to @iamhopaul123's question there - it seems like this is another way of doing the same thing?

I hadn't seen this way of using a pointer, but using this construct instead of two mutually exclusive flags actually simplifies the logic in a pleasant way. This way the behavior is clear and the user is only prompted if they don't specify the flag one way or another via --init-wkld=false or --init-wkld.

bvtujo · 2023-08-21T14:34:04Z

This shouldn't affect svc deploy at all; I tried to scope these changes only to deploy to provide a more guided experience which can also be used in CI.

When a customer does copilot init --deploy we do the following:

prompt them to initialize a service
prompt them to initialize a test environment with limited customization
deploy the environment
deploy the service

With this change to copilot deploy, we do the following:

Check for local manifests and include them in validation
if the service the customer selects via flags or prompting doesn't exist, prompt the customer to initialize it using info gleaned from the local manifest
deploy the service

My next PR will do the same for local environment manifests:

prompt the customer to initialize the environment if a manifest exists and it isn't yet initted
optionally deploy the environment before deploying the service.

iamhopaul123

LGTM overall! just one question.

bvtujo and others added 16 commits July 28, 2023 14:03

chore: add LocalOrConfigWorkload to selector

7fdcb84

chore: refactor to use options pattern in NewLocalWorkloadSelector

4aa5afc

chore: refactor Service and Job selectors to use localOnly logic

fa913a4

refactor: streamline logic in generateOptions and reduce duplication

df2cab9

test: unit test SelectOption

226c239

chore: fix nits

83e4051

chore: refactor deployWkldVars to deployVars

c1b0bdf

chore: allow customizing env name in copilot init

497bd40

chore: remove unnecessary spinner during rendered env deletion

1535843

test: update init e2e

e880955

chore: add selector to init when multiple envs are present

8775332

chore: update behavior to use selectOne whenever any envs are returned

f112fd8

Update e2e/init/init_suite_test.go

147c1b9

Co-authored-by: Penghao He <penghaoh@amazon.com>

chore: add --no-deploy flag to copilot init

eb1ec9a

chore: add new method to initialize to shortcut manifest writing

7ce28f5

chore: add logic to init while deploying

d1ca834

bvtujo requested a review from a team as a code owner August 8, 2023 18:40

bvtujo requested review from Lou1415926 and removed request for a team August 8, 2023 18:40

amazon-ecs-cli-v2-pr-manager requested a review from KollaAdithya August 8, 2023 18:40

huanjani reviewed Aug 11, 2023

View reviewed changes

CaptainCarpensir reviewed Aug 15, 2023

View reviewed changes

Comment thread internal/pkg/cli/init.go

CaptainCarpensir reviewed Aug 15, 2023

View reviewed changes

Comment thread internal/pkg/initialize/workload.go Outdated

CaptainCarpensir reviewed Aug 15, 2023

View reviewed changes

dannyrandall mentioned this pull request Aug 16, 2023

chore: fix apprunner e2e tests #5195

Merged

chore: merge mainline

0b0c4ad

chore: fix nits & staticCheck

e7c8697

chore: use bool pointer for prompting to confirm initialization

9b35f0b

dannyrandall reviewed Aug 21, 2023

View reviewed changes

iamhopaul123 reviewed Aug 21, 2023

View reviewed changes

Comment thread internal/pkg/cli/flag.go Outdated

Comment thread internal/pkg/cli/init.go Outdated

chore: address feedback; remove mutually exclusive flags

735105a

iamhopaul123 approved these changes Aug 22, 2023

View reviewed changes

dannyrandall approved these changes Aug 22, 2023

View reviewed changes

Comment thread internal/pkg/cli/flag.go Outdated

Comment thread internal/pkg/cli/flag.go Outdated

Comment thread internal/pkg/cli/flag.go Outdated

dannyrandall added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Aug 22, 2023

bvtujo added 2 commits August 23, 2023 12:45

chore: nits

6fa2537

Merge branch 'mainline' into chore/init-deploy-logic

6965ff4

bvtujo removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Aug 23, 2023

mergify Bot merged commit 26c79c9 into aws:mainline Aug 23, 2023

		yesInitWorkloadFlag = "init-wkld"
		noInitWorkloadFlag = "no-init-wkld"

Conversation

bvtujo commented Aug 8, 2023

Uh oh!

huanjani Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

huanjani Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

bvtujo Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

iamhopaul123 Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

CaptainCarpensir left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

dannyrandall left a comment

Choose a reason for hiding this comment

Uh oh!

dannyrandall Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

dannyrandall Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

bvtujo Aug 21, 2023

Choose a reason for hiding this comment

Uh oh!

bvtujo commented Aug 21, 2023

Uh oh!

iamhopaul123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

github-actions Bot commented Aug 18, 2023 •

edited

Loading

codecov-commenter commented Aug 18, 2023 •

edited

Loading