feat(cli): support manual approvals in pipeline manifest. #1273

tam0ri · 2020-08-17T03:05:25Z

related #1179

When a production environment is added via pipeline init, copilot adds the requires_approval: true line in pipeline.yml.
When requires_approval: true line exists in pipeline.yml, manual approval action is added into the pipeline via pipeline update.

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

When a production environment is added via pipeline init, copilot adds the requires_approval: true line in pipeline.yml. When requires_approval: true line exists in pipeline.yml, manual approval action is added into the pipeline via pipeline update.

tam0ri · 2020-08-17T03:18:43Z

Hi reviewers. I'm not sure we need to update the version of pipeline manifest.

copilot-cli/internal/pkg/manifest/pipeline.go

Lines 75 to 78 in a145f22

    
           const ( 
        
           	// Ver1 is the current schema major version of the pipeline.yml file. 
        
           	Ver1 PipelineSchemaMajorVersion = iota + 1 
        
           )

copilot-cli/internal/pkg/manifest/pipeline.go

Line 119 in a145f22

Version: Ver1,

If we need to increment the version, please tell me that.

efekarakus

😍 Thank you so much!

It looks good, mostly a comment about decoupling the manifest pkg from config.

efekarakus · 2020-08-17T16:59:29Z

internal/pkg/manifest/pipeline.go

 }

 // CreatePipeline returns a pipeline manifest object.
-func CreatePipeline(pipelineName string, provider Provider, stageNames []string) (*PipelineManifest, error) {
+func CreatePipeline(pipelineName string, provider Provider, envs []*config.Environment) (*PipelineManifest, error) {


nit: Would you mind renaming this function to NewPipelineManifest?

I addressed.

efekarakus · 2020-08-17T17:07:50Z

internal/pkg/cli/pipeline_init.go

+		environments = append(environments, env)
+	}
+
+	manifest, err := manifest.CreatePipeline(pipelineName, provider, environments)


Instead of passing []*config.Environment as input to this function, what do you think of passing []manifest.PipelineStage? so this method would transform []*config.Environment -> []manifest.PipelineStage.

Since the function is in the manifest pkg, accepting []manifest.PipelineStage will make it more flexible. The PipelineStage can be created from a config.Environment object or some other way.

I addressed. However, now TestNewPipelineManifest just become meaningless because this test expects PipelineStage. I plan to change this test such as it expects PipelineManifest. If you have any opinion, please give me a feedback!

hmm yeah I agree, the only behavior that might be interesting to test in TestNewPipelineManifest is checking if an error is returned if the list is empty and that's it

Oh, does it mean we simply need to remove happy case and retain only error case? I just modified happy case with PipelineManifest as expected by imitating backend_svc_test.go. If I need to remove happy case, please point out!

efekarakus · 2020-08-17T17:08:09Z

internal/pkg/cli/pipeline_init.go

@@ -247,14 +247,36 @@ func (o *initPipelineOpts) createPipelineProvider() (manifest.Provider, error) {
 	return manifest.NewProvider(config)
 }

+func (o *initPipelineOpts) getEnvFromCache(environmentName string) (*config.Environment, error) {


nit: Can we rename this method to getEnvConfig?

I addressed.

efekarakus · 2020-08-17T17:08:32Z

internal/pkg/cli/pipeline_init.go

+	var environment *config.Environment
+	for _, env := range o.envs {
+		if env.Name == environmentName {
+			environment = env


nit: break after this statement so that we don't continue looping.

I addressed.

nit: oh my bad, instead of breaking we should just return instead :)

and then outside of the for-loop we wouldn't need to check if it's equal to nil and return also an error immediately

Totally agree! I addressed.

efekarakus · 2020-08-17T17:12:02Z

👋 @git823

If we need to increment the version, please tell me that.

We can keep it as is. We don't do anything with the version atm, so it should be fine :)

efekarakus · 2020-08-17T17:19:46Z

This will be a breaking-change for customers that have a pipeline with environments that are initialized with --prod. When we unmarshal the manifest, the requires_approval field won't exist and the copilot pipeline update cmd will result in the removal of the manual approval.

I think this is fine though, we'll explicitly call it out in our release notes as this change gives the customers power to control the manual approvals :)

Rename getEnvFromCache -> getEnvConfig. Add break statement.

Rename CreatePipeline -> NewPipelineManifest. Instead of passing []*config.Environment as input to NewPipelineManifest, passing []manifest.PipelineStage.

efekarakus · 2020-08-17T21:07:09Z

internal/pkg/cli/pipeline_init.go

+	var environment *config.Environment
+	for _, env := range o.envs {
+		if env.Name == environmentName {
+			environment = env


nit: oh my bad, instead of breaking we should just return instead :)

kohidave · 2020-08-17T21:48:37Z

templates/cicd/pipeline.yml

@@ -21,6 +21,8 @@ source:
 stages:{{range .Stages}}
    - # The name of the environment to deploy to.
      name: {{.Name}}
+      # Optional: flag for manual approval action before deployment.
+      requires_approval: {{.RequiresApproval}}


What do you think of having this commented out, by default (like the test commands)? It will be false by default. and folks can uncomment it if they want it!

Sounds nice! I addressed.

efekarakus

🚀 Looks great! Thank you

related aws#1179 When a production environment is added via `pipeline init`, copilot adds the requires_approval: true line in pipeline.yml. When requires_approval: true line exists in pipeline.yml, manual approval action is added into the pipeline via `pipeline update`. By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

tam0ri requested a review from a team as a code owner August 17, 2020 03:05

tam0ri requested a review from kohidave August 17, 2020 03:05

amazon-ecs-cli-v2-pr-manager requested a review from huanjani August 17, 2020 03:05

efekarakus reviewed Aug 17, 2020

View reviewed changes

tam0ri added 4 commits August 18, 2020 05:01

Modify getEnvFromCache function.

7eed482

Rename getEnvFromCache -> getEnvConfig. Add break statement.

Rename test function name

16d65b3

Adress feedback

ac8c948

Rename CreatePipeline -> NewPipelineManifest. Instead of passing []*config.Environment as input to NewPipelineManifest, passing []manifest.PipelineStage.

Remove code added intentionally in previous commit.

1f49663

efekarakus approved these changes Aug 17, 2020

View reviewed changes

kohidave approved these changes Aug 17, 2020

View reviewed changes

efekarakus added the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Aug 17, 2020

tam0ri added 3 commits August 19, 2020 04:54

Modify getEnvConfig function.

6873961

Modify pipeline.yml.

09155fc

Modify TestNewPipelineManifest.

69dac59

efekarakus approved these changes Aug 18, 2020

View reviewed changes

kohidave removed the do-not-merge Pull requests that mergify shouldn't merge until the requester allows it. label Aug 18, 2020

Merge branch 'mainline' into feat/pipeline-approval

effe033

mergify bot merged commit e096cf8 into aws:mainline Aug 18, 2020

tam0ri deleted the feat/pipeline-approval branch August 19, 2020 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): support manual approvals in pipeline manifest. #1273

feat(cli): support manual approvals in pipeline manifest. #1273

tam0ri commented Aug 17, 2020 •

edited

Loading

tam0ri commented Aug 17, 2020

efekarakus left a comment

efekarakus Aug 17, 2020

tam0ri Aug 17, 2020

efekarakus Aug 17, 2020

tam0ri Aug 17, 2020

efekarakus Aug 17, 2020

tam0ri Aug 18, 2020

efekarakus Aug 17, 2020

tam0ri Aug 17, 2020

efekarakus Aug 17, 2020

tam0ri Aug 17, 2020

efekarakus Aug 17, 2020

efekarakus Aug 17, 2020

tam0ri Aug 18, 2020

efekarakus commented Aug 17, 2020

efekarakus commented Aug 17, 2020 •

edited

Loading

efekarakus Aug 17, 2020

kohidave Aug 17, 2020

tam0ri Aug 18, 2020

efekarakus left a comment

feat(cli): support manual approvals in pipeline manifest. #1273

feat(cli): support manual approvals in pipeline manifest. #1273

Conversation

tam0ri commented Aug 17, 2020 • edited Loading

tam0ri commented Aug 17, 2020

efekarakus 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

efekarakus commented Aug 17, 2020

efekarakus commented Aug 17, 2020 • edited Loading

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

Choose a reason for hiding this comment

tam0ri commented Aug 17, 2020 •

edited

Loading

efekarakus commented Aug 17, 2020 •

edited

Loading