Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Canary & Blue/Green for ECS by pipectl init #4801

Merged
merged 22 commits into from
Apr 16, 2024

Conversation

t-kikuc
Copy link
Member

@t-kikuc t-kikuc commented Mar 1, 2024

What this PR does / why we need it:

[1] Support generating app.pipecd.yaml by pipectl init for ECS with

  • Canary
  • Blue/Green

[2] Remove required input fields to simplify user's input

Which issue(s) this PR fixes:

Fixes #4800

Does this PR introduce a user-facing change?: no

  • How are users affected by this change: no
  • Is this breaking change: no
  • How to migrate (if breaking change): no

Signed-off-by: t-kikuc <[email protected]>
Signed-off-by: t-kikuc <[email protected]>
Signed-off-by: t-kikuc <[email protected]>
Signed-off-by: t-kikuc <[email protected]>
Copy link

codecov bot commented Mar 1, 2024

Codecov Report

Attention: Patch coverage is 88.34081% with 26 lines in your changes are missing coverage. Please review.

Project coverage is 29.16%. Comparing base (4436527) to head (4429f42).
Report is 1 commits behind head on master.

Files Patch % Lines
pkg/app/pipectl/cmd/initialize/ecs.go 89.14% 16 Missing and 8 partials ⚠️
pkg/config/percentage.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4801      +/-   ##
==========================================
+ Coverage   28.91%   29.16%   +0.24%     
==========================================
  Files         317      317              
  Lines       40369    40558     +189     
==========================================
+ Hits        11674    11828     +154     
- Misses      27767    27793      +26     
- Partials      928      937       +9     

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

@t-kikuc
Copy link
Member Author

t-kikuc commented Mar 2, 2024

/review

Copy link
Contributor

github-actions bot commented Mar 2, 2024

PR Analysis

Main theme

Enhancement of the ECS application configuration initialization process

PR summary

This PR enhances the `pipectl init` command for ECS applications by introducing interactive prompts for user input, adding support for different deployment strategies (QuickSync, Canary, BlueGreen), and accommodating target group configurations. The code changes include the redefinition of input variables and prompt structuring, along with the generation of an application configuration YAML file based on the provided input.

Type of PR

Enhancement

PR Feedback:

General suggestions

Overall, the PR reflects a significant effort to improve the user experience when initializing ECS application configurations. It will likely make the process easier and clearer for users, especially with the direct support for different deployment strategies. However, ensuring that default values for prompts are explicitly handled and refining some parts of the code to avoid duplication will further enhance this PR.

Code feedback

- relevant file: `pkg/app/pipectl/cmd/initialize/ecs.go`
  suggestion: Consider assigning default prompt values to a constant since they are used multiple times. Duplicate literals make the process of changing a value prone to errors. (important)
  relevant line: `+		targetGroupArn string = "<YOUR_TARGET_GROUP_ARN>"`

- relevant file: `pkg/app/pipectl/cmd/initialize/ecs.go`
  suggestion: Consolidate the handling of the default deployment strategy. The prompt's default value and the switch case's default should be in sync to avoid misunderstandings. (important)
  relevant line: `+		deploymentStrategy string = "0" // QuickSync by default`

- relevant file: `pkg/app/pipectl/cmd/initialize/ecs.go`
  suggestion: Add comments to outline the structure of the deployment pipeline stages. As this is crucial for understanding the deployment strategy, comments will help maintainers and contributors understand the code faster. (medium)
  relevant line: `+				{`

- relevant file: `pkg/app/pipectl/cmd/initialize/ecs.go`
  suggestion: In the `inputECSBlueGreen` function, the `Number: 100` setting is duplicated for `ECSCanaryRolloutStageOptions` and `ECSTrafficRoutingStageOptions`. Consider abstracting this to a variable at the top of the function to avoid magic numbers and facilitate future changes. (medium)
  relevant line: `+						Number: 100,`

Security concerns:

no

The PR code does not introduce direct security concerns as it mainly pertains to command-line prompts and configuration generation, without handling external inputs that could be source vectors for attacks like SQL injection or CSRF. It's essential to ensure that future changes do not allow external influence on the prompt input, which could raise security issues.

@t-kikuc t-kikuc removed their assignment Mar 13, 2024
Copy link
Member

@khanhtc1202 khanhtc1202 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, nice improvement 👍

Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 👌

@ffjlabo ffjlabo merged commit a86b88e into master Apr 16, 2024
14 checks passed
@ffjlabo ffjlabo deleted the pipectl-init-ecs-canary branch April 16, 2024 02:04
@github-actions github-actions bot mentioned this pull request Apr 22, 2024
khanhtc1202 added a commit that referenced this pull request Apr 22, 2024
* Add expected YAML for kustomize

Signed-off-by: t-kikuc <[email protected]>

* Add Kustomize pattern for pipectl init

Signed-off-by: t-kikuc <[email protected]>

* Add Helm pattern for pipectl init

Signed-off-by: t-kikuc <[email protected]>

* Add options for Helm

Signed-off-by: t-kikuc <[email protected]>

* fix typo

Signed-off-by: t-kikuc <[email protected]>

* Add ECS canary for pipectl init

Signed-off-by: t-kikuc <[email protected]>

* Add pipectl init status

Signed-off-by: t-kikuc <[email protected]>

* add init cmd to pipectl doc

Signed-off-by: t-kikuc <[email protected]>

* add pipectl init explanation

Signed-off-by: t-kikuc <[email protected]>

* fi pipectl init status

Signed-off-by: t-kikuc <[email protected]>

* Embed the simple AnalysisStage in pipeline by default

Signed-off-by: t-kikuc <[email protected]>

* Merge fix origin/master into pipectl-init-ecs-canary

Signed-off-by: t-kikuc <[email protected]>

* fix prompt message and default value

Signed-off-by: t-kikuc <[email protected]>

* Fix output YAML structure by generic structs

Signed-off-by: t-kikuc <[email protected]>

* Support Blue/Green for ECS by pipectl init

Signed-off-by: t-kikuc <[email protected]>

* omitempty Percentage

Signed-off-by: t-kikuc <[email protected]>

* Changed default values for simpler config

Signed-off-by: t-kikuc <[email protected]>

* Rename funcs to avoid name conflicts with other platforms

Signed-off-by: t-kikuc <[email protected]>

---------

Signed-off-by: t-kikuc <[email protected]>
Co-authored-by: Khanh Tran <[email protected]>
khanhtc1202 added a commit that referenced this pull request Apr 22, 2024
* Add expected YAML for kustomize

Signed-off-by: t-kikuc <[email protected]>

* Add Kustomize pattern for pipectl init

Signed-off-by: t-kikuc <[email protected]>

* Add Helm pattern for pipectl init

Signed-off-by: t-kikuc <[email protected]>

* Add options for Helm

Signed-off-by: t-kikuc <[email protected]>

* fix typo

Signed-off-by: t-kikuc <[email protected]>

* Add ECS canary for pipectl init

Signed-off-by: t-kikuc <[email protected]>

* Add pipectl init status

Signed-off-by: t-kikuc <[email protected]>

* add init cmd to pipectl doc

Signed-off-by: t-kikuc <[email protected]>

* add pipectl init explanation

Signed-off-by: t-kikuc <[email protected]>

* fi pipectl init status

Signed-off-by: t-kikuc <[email protected]>

* Embed the simple AnalysisStage in pipeline by default

Signed-off-by: t-kikuc <[email protected]>

* Merge fix origin/master into pipectl-init-ecs-canary

Signed-off-by: t-kikuc <[email protected]>

* fix prompt message and default value

Signed-off-by: t-kikuc <[email protected]>

* Fix output YAML structure by generic structs

Signed-off-by: t-kikuc <[email protected]>

* Support Blue/Green for ECS by pipectl init

Signed-off-by: t-kikuc <[email protected]>

* omitempty Percentage

Signed-off-by: t-kikuc <[email protected]>

* Changed default values for simpler config

Signed-off-by: t-kikuc <[email protected]>

* Rename funcs to avoid name conflicts with other platforms

Signed-off-by: t-kikuc <[email protected]>

---------

Signed-off-by: t-kikuc <[email protected]>
Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: khanhtc1202 <[email protected]>
khanhtc1202 added a commit that referenced this pull request Apr 22, 2024
#4883 #4887 #4885 #4886 #4884 #4880 (#4890)

* Update contributors list (#4866)

Signed-off-by: khanhtc1202 <[email protected]>

* Use valid semver in run/pipecd command (#4851)

* Use valid semver in run/pipecd command

Signed-off-by: David <[email protected]>

* Swapped to simpler local version

Signed-off-by: David Gannon <[email protected]>

* Include a comment explaining why we hard coded

Signed-off-by: David Gannon <[email protected]>

---------

Signed-off-by: David <[email protected]>
Signed-off-by: David Gannon <[email protected]>
Signed-off-by: khanhtc1202 <[email protected]>

* Support Canary & Blue/Green for ECS by `pipectl init` (#4801)

* Add expected YAML for kustomize

Signed-off-by: t-kikuc <[email protected]>

* Add Kustomize pattern for pipectl init

Signed-off-by: t-kikuc <[email protected]>

* Add Helm pattern for pipectl init

Signed-off-by: t-kikuc <[email protected]>

* Add options for Helm

Signed-off-by: t-kikuc <[email protected]>

* fix typo

Signed-off-by: t-kikuc <[email protected]>

* Add ECS canary for pipectl init

Signed-off-by: t-kikuc <[email protected]>

* Add pipectl init status

Signed-off-by: t-kikuc <[email protected]>

* add init cmd to pipectl doc

Signed-off-by: t-kikuc <[email protected]>

* add pipectl init explanation

Signed-off-by: t-kikuc <[email protected]>

* fi pipectl init status

Signed-off-by: t-kikuc <[email protected]>

* Embed the simple AnalysisStage in pipeline by default

Signed-off-by: t-kikuc <[email protected]>

* Merge fix origin/master into pipectl-init-ecs-canary

Signed-off-by: t-kikuc <[email protected]>

* fix prompt message and default value

Signed-off-by: t-kikuc <[email protected]>

* Fix output YAML structure by generic structs

Signed-off-by: t-kikuc <[email protected]>

* Support Blue/Green for ECS by pipectl init

Signed-off-by: t-kikuc <[email protected]>

* omitempty Percentage

Signed-off-by: t-kikuc <[email protected]>

* Changed default values for simpler config

Signed-off-by: t-kikuc <[email protected]>

* Rename funcs to avoid name conflicts with other platforms

Signed-off-by: t-kikuc <[email protected]>

---------

Signed-off-by: t-kikuc <[email protected]>
Co-authored-by: Khanh Tran <[email protected]>
Signed-off-by: khanhtc1202 <[email protected]>

* [doc] Fix typo x2 in DeploymentChain (#4872)

* Fix typo: archive->achieve

Signed-off-by: t-kikuc <[email protected]>

* Fix type: deployment->development

Signed-off-by: t-kikuc <[email protected]>

* Fix typos of 'achieve' and 'under development' in older docs

Signed-off-by: t-kikuc <[email protected]>

---------

Signed-off-by: t-kikuc <[email protected]>
Signed-off-by: khanhtc1202 <[email protected]>

* docs: added install method (#4875)

Signed-off-by: khanhtc1202 <[email protected]>

* Fix unable to use SecretEncryption and Attachment features at the same time (#4855)

* Add test to mention error with go templating

Signed-off-by: khanhtc1202 <[email protected]>

* Reimplement sourceprosser logic

Signed-off-by: khanhtc1202 <[email protected]>

* SourceProssesor without processor should be marked as error

Signed-off-by: khanhtc1202 <[email protected]>

* Add test

Signed-off-by: khanhtc1202 <[email protected]>

* Enable perform template processing in chain

Signed-off-by: khanhtc1202 <[email protected]>

* Fix typo

Signed-off-by: khanhtc1202 <[email protected]>

---------

Signed-off-by: khanhtc1202 <[email protected]>

* Add a caution comment of scrape_interval (#4869)

* Add caution comment of scrape_interval

Signed-off-by: t-kikuc <[email protected]>

* Remove concrete value

Signed-off-by: t-kikuc <[email protected]>

* Fix to commit hash of v0.47.0 to fix the ref position

Signed-off-by: t-kikuc <[email protected]>

---------

Signed-off-by: t-kikuc <[email protected]>
Signed-off-by: khanhtc1202 <[email protected]>

* Rewrite pipectl installation docs (#4877)

Signed-off-by: khanhtc1202 <[email protected]>

* Fix typo (#4878)

Signed-off-by: khanhtc1202 <[email protected]>

* Set initial-branch on git init (#4882)

Signed-off-by: t-kikuc <[email protected]>
Signed-off-by: khanhtc1202 <[email protected]>

* Update contributors list (#4883)

Signed-off-by: t-kikuc <[email protected]>
Signed-off-by: khanhtc1202 <[email protected]>

* Bump golang.org/x/net from 0.17.0 to 0.23.0 (#4887)

Signed-off-by: khanhtc1202 <[email protected]>

* Bump golang.org/x/net in /tool/actions-plan-preview (#4885)

Signed-off-by: khanhtc1202 <[email protected]>

* Bump golang.org/x/net from 0.17.0 to 0.23.0 in /tool/actions-gh-release (#4886)

Signed-off-by: khanhtc1202 <[email protected]>

* Update default versions of kubectl, kustomize and helm in configuration-reference.md (#4884)

Signed-off-by: khanhtc1202 <[email protected]>

* Add Homebrew Formula (#4880)

Signed-off-by: khanhtc1202 <[email protected]>

---------

Signed-off-by: khanhtc1202 <[email protected]>
Signed-off-by: David <[email protected]>
Signed-off-by: David Gannon <[email protected]>
Signed-off-by: t-kikuc <[email protected]>
Co-authored-by: David Gannon <[email protected]>
Co-authored-by: Tetsuya Kikuchi <[email protected]>
Co-authored-by: YuyaKoda <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Shinichi Nishimura <[email protected]>
Co-authored-by: Shohei Ueda <[email protected]>
@github-actions github-actions bot mentioned this pull request May 13, 2024
@github-actions github-actions bot mentioned this pull request Jun 12, 2024
@github-actions github-actions bot mentioned this pull request Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support canary for ECS by pipectl init
4 participants