Create pipectl init for ECS #4741

t-kikuc · 2023-12-26T07:33:54Z

What this PR does / why we need it:

Create pipectl init command for ECS's simplest use case.
I'll add Kubernetes next.

pipectl-init-ecs-1.mov

(x1.5 pace)

NOTE: This PR depends on #4737

Which issue(s) this PR fixes:

Part of #4677

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

t-kikuc · 2023-12-26T07:36:33Z

I'll clean unnecessary commits

codecov · 2023-12-26T07:38:08Z

Codecov Report

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

Comparison is base (522a13b) 30.87% compared to head (2a4339a) 31.03%.

Files	Patch %	Lines
pkg/app/pipectl/cmd/initialize/initialize.go	0.00%	92 Missing ⚠️
pkg/app/pipectl/cmd/initialize/prompt/prompt.go	81.35%	10 Missing and 1 partial ⚠️
pkg/config/config.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4741      +/-   ##
==========================================
+ Coverage   30.87%   31.03%   +0.15%     
==========================================
  Files         222      225       +3     
  Lines       26037    26257     +220     
==========================================
+ Hits         8039     8148     +109     
- Misses      17349    17458     +109     
- Partials      649      651       +2

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

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

t-kikuc · 2023-12-26T09:57:24Z

I'll clean unnecessary commits

Done

t-kikuc · 2023-12-26T09:57:31Z

/review

github-actions · 2023-12-26T09:58:47Z

PR Analysis

Main theme

Enhancement

PR summary

This PR introduces initialization commands to assist users in generating application configuration files for ECS applications in an interactive manner.

Type of PR

Enhancement

PR Feedback:

General suggestions

The new code implements functionality that helps users generate application configurations through an interactive command-line interface. This is a user-friendly feature, which assists in setting up application configurations without needing in-depth knowledge about the required fields or the structure of the configuration itself.
It is crucial to consider input validation and error handling to ensure that the tool provides meaningful feedback for incorrect inputs and to prevent any potential issues from misconfiguration.
Refactoring some duplicated logic into reusable functions could improve maintainability and testability.

Code feedback

relevant file: pkg/app/pipectl/cmd/initialize/ecs.go
suggestion: |-
It would be beneficial to encapsulate the user input prompts into separate functions to promote code reuse and improve testability. For example, prompting and reading the service definition file can be refactored into a reusable function. Since this is a non-critical enhancement, it can be treated as a 'medium' priority.
```
func promptServiceDefinitionFile(in io.Reader) string {
  return promptStringRequired("Name of the service definition file (e.g. serviceDef.yaml): ", in)
}
```
relevant line: + serviceDefFile := promptStringRequired("Name of the service definition file (e.g. serviceDef.yaml): ", in)
relevant file: pkg/app/piped/platformprovider/ecs/target_groups.go
suggestion: |-
Consider adding more comprehensive validation of the TargetGroupArn, ContainerName, and ContainerPort values to ensure that they meet AWS's requirements. This could help in catching misconfigurations early in the setup process, rather than at runtime. Tagging with 'important' due to the potential impact on deployment reliability.
```
if err := validateTargetGroup(primary); err != nil {
  return nil, nil, err
}
```
relevant line: + primary := &types.LoadBalancer{

Security concerns:

No

The changes seem to focus on configuration generation and input handling, which does not introduce security concerns such as SQL injection, XSS, CSRF, etc.

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

ffjlabo

Thank you for your great contribution!
I left a comment at first 👍

ffjlabo

oops... I forgot to add review comment.
re: comment🙏

ffjlabo · 2024-01-05T11:05:14Z

pkg/app/pipectl/prompt/reader.go

+// Reader is an interface that reads input from user or mock.
+type Reader interface {
+	// ReadString reads a string with showing message. If the input is empty, it returns an empty string.
+	// If the input is more than one word, it returns an error.
+	ReadString(message string) (string, error)
+	// ReadStrings reads a string slice separated by blank space with showing message. If the input is empty, it returns an empty slice.
+	ReadStrings(message string) ([]string, error)
+	// ReadInt reads an integer with showing message. If the input is empty, it returns 0.
+	// If the input is not an integer, it returns an error.
+	ReadInt(message string) (int, error)
+	// ReadStringRequired reads a string with showing message. If the input is empty, it asks again.
+	// If number of retry exceeds maximum_retry, it returns an error.
+	// If the input is more than one word, it returns an error.
+	ReadStringRequired(message string) (string, error)
+}


[ask] What's the purpose of the Interface?
I think reading data from stdin is a concrete usecase, so it might not be necessary abstraction 👀
WDYT?

The purpose is to use mock in initialize/ecs_test.go.
In tests, stdin is difficult to use but we need to pass values instead of users.
So in tests I use mockReader(in prompt/reader_mock.go), which implements Reader.

Thx, I got it!
So you can create prompt.NewReader(input io.Reader) instead of NewStdinReader and mock the input 👍
You can create the implementation of io.Reader from a string using strings.NewReader https://pkg.go.dev/strings#NewReader

Like this ↓

// default r := prompt.NewReader(os.Stdin) // when test mockStr := `foo foo bar ` mockStdin := strings.NewReader(mockStr) r := prompt.NewReader(mockStdin)

@ffjlabo
great, amazing, thank you so much!
It worked well, and I could remove the Reader interface!
strings.NewReader is what i wanted.

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

ffjlabo

Thank you for the fixes!
Added some more comments 🙏

ffjlabo · 2024-01-10T05:15:23Z

pkg/config/application_ecs.go

 	// Automatically reverts all changes from all stages when one of them failed.
 	// Default is true.
 	AutoRollback *bool `json:"autoRollback,omitempty" default:"true"`
 	// Run standalone task during deployment.
 	// Default is true.
-	RunStandaloneTask *bool `json:"runStandaloneTask" default:"true"`
+	RunStandaloneTask *bool `json:"runStandaloneTask,omitempty" default:"true"`


[must] look back whether to check the reference of the field is nil before using it for avoiding nil pointer exception.

Thanks, I confirmed there's no preblem.
omitempty affects only encoding to yaml/json, not decoding from yaml/json.
And when encoding,

If RunStandaloneTask is not defined in genericECSApplicationSpec.Input, runStandaloneTask won't appear in the pipectl init output.

If RunStandaloneTask has null value, runStandaloneTask: false will appear in the pipectl init output.

ffjlabo · 2024-01-10T05:16:49Z

pkg/app/pipectl/cmd/initialize/ecs.go

@@ -0,0 +1,90 @@
+// Copyright 2023 The PipeCD Authors.


[must] plz update the copyright to the one of 2024

Thanks, I updated all

ffjlabo · 2024-01-10T05:31:29Z

pkg/app/pipectl/cmd/initialize/ecs_test.go

+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {


[nit] add tc := tc

Thanks, I fixed with adding T.Parallel()

ffjlabo · 2024-01-10T06:02:02Z

pkg/app/pipectl/cmd/initialize/initialize.go

+	switch platform {
+	case "0": // Kubernetes
+		// cfg, err = generateKubernetesConfig(in)
+		panic("not implemented")
+	case "1": // ECS
+		cfg, err = generateECSConfig(reader)


[imo] Define identifier for the platform number as the const value for the readability of the code👍

const ( PlatformKubernetes string = "0" PlatformECS string = "1" ) switch platform { case PlatformKubernetes: case PlatformECS: }

Thanks, that's better

ffjlabo · 2024-01-10T07:20:33Z

pkg/app/pipectl/prompt/reader.go

+func (r *Reader) ReadStringRequired(message string) (string, error) {
+	for i := 0; i < maximum_retry; i++ {
+		s, e := r.ReadString(message)
+		if e != nil {
+			return "", e
+		}
+		if len(s) > 0 {
+			return s, nil
+		}
+		fmt.Printf("[WARN] This field is required. \n")
+	}
+	return "", fmt.Errorf("this field is required. Maximum retry(%d) exceeded", maximum_retry)


[imo] How about adding the argument required bool to each method like ReadStrings and ReadString instead of preparing the method for required input.

Thanks, I created prompt.Input.Required

ffjlabo · 2024-01-10T09:34:21Z

pkg/app/pipectl/cmd/initialize/ecs.go

+	spec, e := generateECSSpec(reader)
+	if e != nil {
+		return nil, e
+	}


[imo] It would be nice to define necessary values first, and pass them to generateECSSpec 👀 WDYT?

Thanks, I refactored

ffjlabo · 2024-01-10T09:35:27Z

pkg/app/pipectl/cmd/initialize/ecs.go

+}
+
+func generateECSSpec(reader prompt.Reader) (*genericECSApplicationSpec, error) {
+	appName, e := reader.ReadStringRequired("Name of the application: ")


[must] Use err as the variable for error instead of e. Go often uses err 👍

Thanks, I fixed all

ffjlabo · 2024-01-10T09:37:23Z

pkg/app/pipectl/prompt/reader.go

+// Reader reads input from user or mock.
+type Reader struct {
+	bufReader bufio.Reader
+}
+
+func NewReader(in io.Reader) Reader {
+	return Reader{
+		bufReader: *bufio.NewReader(in),
+	}
+}
+
+// ReadString reads a string with showing message. If the input is empty, it returns an empty string.
+// If the input is more than one word, it returns an error.
+func (r *Reader) ReadString(message string) (string, error) {
+	s, e := r.ReadStrings(message)
+	if e != nil {
+		return "", e
+	}
+
+	if len(s) == 0 {
+		return "", nil
+	}
+	if len(s) > 1 {
+		return "", fmt.Errorf("too many arguments")
+	}
+
+	return s[0], nil
+}


[imo] How about rename like below? WDYT?

Reader -> Prompt

ReadXX -> InputXX

Thanks, I renamed

Reader -> Prompt

ReadXX -> removed and created prompt.Input struct.

ffjlabo · 2024-01-10T09:40:19Z

pkg/app/pipectl/cmd/initialize/ecs_test.go

+		{
+			name: "valid config for ECSApp",
+			inputs: `myApp
+				serviceDef.yaml
+				taskDef.yaml
+				arn:aws:elasticloadbalancing:ap-northeast-1:123456789012:targetgroup/xxx/xxx
+				web
+				80
+				`,
+			expectedFile: "testdata/ecs-app.yaml",
+			expectedErr:  nil,
+		},


[imo] Add testcase for failed case 👀

Thanks, I added a failure case.

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

t-kikuc · 2024-01-11T08:41:31Z

@ffjlabo Thank you for great reviewing. I fixed all

ffjlabo · 2024-01-18T08:47:05Z

pkg/app/pipectl/prompt/prompt.go

+type Prompt struct {
+	bufReader bufio.Reader
+}
+
+type Input struct {
+	Message       string
+	TargetPointer any
+	Required      bool
+}


[nit] Add comments for these structs and fields 👍

ffjlabo · 2024-01-18T08:49:32Z

pkg/app/pipectl/prompt/prompt_test.go

+		t.Run(tc.name, func(t *testing.T) {
+			strReader := strings.NewReader(tc.str)
+			p := NewPrompt(strReader)


[nit] Add t.Parallel here :)

ffjlabo · 2024-01-18T08:50:03Z

pkg/app/pipectl/prompt/prompt_test.go

+		t.Run(tc.name, func(t *testing.T) {
+			strReader := strings.NewReader(tc.str)
+			p := NewPrompt(strReader)


[nit] Add t.Parallel here :)

Same as above.

ffjlabo · 2024-01-18T08:51:00Z

pkg/app/pipectl/prompt/prompt_test.go

+		t.Run(tc.name, func(t *testing.T) {
+			strReader := strings.NewReader(tc.str)
+			p := NewPrompt(strReader)


[nit] Add t.Parallel here :)

Same as above.

ffjlabo · 2024-01-18T08:51:22Z

pkg/app/pipectl/prompt/prompt_test.go

+		t.Run(tc.name, func(t *testing.T) {
+			strReader := strings.NewReader(tc.str)
+			p := NewPrompt(strReader)


[nit] Add t.Parallel here :)

Same as above.

ffjlabo · 2024-01-18T08:54:45Z

pkg/app/pipectl/prompt/prompt.go

+}
+
+// RunSlice sequentially asks for inputs and set the values to the target pointers.
+func (prompt *Prompt) RunSlice(inputs []Input) error {


[imo] It should change prompt to p to distinguish package name and receiver name 👍

ffjlabo · 2024-01-18T08:56:11Z

pkg/app/pipectl/cmd/initialize/initialize.go

+const (
+	platformKubernetes string = "0"
+	platformECS        string = "1"
+)


[nit] Add comments :)

ffjlabo · 2024-01-18T09:14:11Z

@t-kikuc
[imo] As for package prompt and exporter, just add prompt.go and exporter.go into pkg/app/pipectl/cmd/initialize 👀
This is because these functionalities are only for pipectl init at this time :)

…ECS (#4746) Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: t-kikuc <[email protected]>

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

…tl-init-ecs Signed-off-by: t-kikuc <[email protected]>

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

t-kikuc · 2024-01-19T05:25:16Z

@t-kikuc [imo] As for package prompt and exporter, just add prompt.go and exporter.go into pkg/app/pipectl/cmd/initialize 👀 This is because these functionalities are only for pipectl init at this time :)

I got it.
I'd use them in pipectl migrate command but that is not definitive.
So they should be in pkg/app/pipectl/cmd/initialize for now.

t-kikuc · 2024-01-19T05:26:20Z

@ffjlabo
Thank you so much, I fixed.

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

ffjlabo

Thank you! 🚀

khanhtc1202

Let's go 🚀

t-kikuc requested review from nghialv, khanhtc1202, kentakozuka and ffjlabo as code owners December 26, 2023 07:33

github-actions bot added the area/go label Dec 26, 2023

t-kikuc force-pushed the pipectl-init branch from 016f930 to 7f89bab Compare December 26, 2023 07:34

github-actions bot added the area/examples label Dec 26, 2023

t-kikuc added 7 commits December 26, 2023 18:53

Add pipectl init command but not implemented

c24329a

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

Modify init command

6170b7d

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

Rename init command to initialize

30f7554

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

Create pipectl int for ECS

419aa6b

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

Add init for ECS

546e2cd

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

Convert json.RawMessage to struct

4141bbf

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

Use original structs of ECS for pipectl init

d877787

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

t-kikuc force-pushed the pipectl-init branch from 7f89bab to d877787 Compare December 26, 2023 09:55

github-actions bot removed the area/examples label Dec 26, 2023

khanhtc1202 added the do-not-merge/hold label Dec 26, 2023

Init k8s app for pipectl init, but not implemented

4097fb7

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

t-kikuc added the do-not-merge/wip label Dec 26, 2023

t-kikuc added 6 commits December 27, 2023 11:06

Fix init input(wip)

e4d71d2

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

Add mock prompt reader

608c91e

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

Reractor prompt reader

a819e97

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

Modify mock reader to public

3e813b7

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

Refactored prompt reader

c9ef764

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

Add yaml snapshot test

6b18136

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

ffjlabo reviewed Jan 5, 2024

View reviewed changes

t-kikuc added 3 commits January 9, 2024 14:52

Use Reader in tests

02cd0bf

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

Remove stdinReader

cb6dab2

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

Remove mockReader

bd35685

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

ffjlabo reviewed Jan 10, 2024

View reviewed changes

t-kikuc added 2 commits January 11, 2024 01:12

Add Prompt and PromptInput types

4813398

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

Remove promptReader

4800de6

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

t-kikuc added the do-not-merge/wip label Jan 10, 2024

t-kikuc added 3 commits January 11, 2024 15:57

Refactored init,exporter

8081826

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

Update Copyright

77c8c92

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

Refactored nits

76f89d0

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

t-kikuc removed the do-not-merge/wip label Jan 11, 2024

ffjlabo reviewed Jan 18, 2024

View reviewed changes

ffjlabo and others added 5 commits January 19, 2024 12:40

Add reference to the blog that shows how to install control plane on …

cf7dfa4

…ECS (#4746) Signed-off-by: Yoshiki Fujikane <[email protected]> Signed-off-by: t-kikuc <[email protected]>

Bump follow-redirects from 1.15.2 to 1.15.4 in /web (#4747)

8b95253

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

Add RC Release Procedure (#4749)

e797d9b

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

Merge branch 'master' of https://github.com/pipe-cd/pipecd into pipec…

2714de4

…tl-init-ecs Signed-off-by: t-kikuc <[email protected]>

Fix nits

85814be

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

Fix error variable name

2a4339a

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

ffjlabo approved these changes Jan 19, 2024

View reviewed changes

khanhtc1202 approved these changes Jan 19, 2024

View reviewed changes

t-kikuc merged commit 0189b18 into master Jan 19, 2024
14 checks passed

t-kikuc deleted the pipectl-init branch January 19, 2024 06:08

t-kikuc self-assigned this Jan 19, 2024

github-actions bot mentioned this pull request Feb 6, 2024

Release v0.46.0 #4775

Merged

		for _, tc := range testcases {
		t.Run(tc.name, func(t *testing.T) {

Create pipectl init for ECS #4741

Create pipectl init for ECS #4741

Conversation

t-kikuc commented Dec 26, 2023 • edited Loading

t-kikuc commented Dec 26, 2023

codecov bot commented Dec 26, 2023 • edited Loading

Codecov Report

t-kikuc commented Dec 26, 2023

t-kikuc commented Dec 26, 2023

github-actions bot commented Dec 26, 2023

PR Analysis

Main theme

PR summary

Type of PR

PR Feedback:

General suggestions

Code feedback

Security concerns:

ffjlabo left a comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo 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

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

t-kikuc commented Jan 11, 2024

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

ffjlabo commented Jan 18, 2024 • edited Loading

t-kikuc commented Jan 19, 2024

t-kikuc commented Jan 19, 2024

ffjlabo left a comment

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

t-kikuc commented Dec 26, 2023 •

edited

Loading

codecov bot commented Dec 26, 2023 •

edited

Loading

ffjlabo Jan 8, 2024 •

edited

Loading

ffjlabo commented Jan 18, 2024 •

edited

Loading