add egctl edit, egctl logs, update egctl create cmd #1067

suchen-sci · 2023-08-18T09:36:14Z

No description provided.

codecov-commenter · 2023-08-18T09:45:40Z

Codecov Report

Patch coverage: 83.00% and project coverage change: -0.07% ⚠️

Comparison is base (fbb4f90) 81.47% compared to head (2ad235c) 81.41%.
Report is 3 commits behind head on main.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1067      +/-   ##
==========================================
- Coverage   81.47%   81.41%   -0.07%     
==========================================
  Files         136      141       +5     
  Lines       15404    15710     +306     
==========================================
+ Hits        12551    12790     +239     
- Misses       2276     2331      +55     
- Partials      577      589      +12

Files Changed	Coverage Δ
cmd/client/commandv2/create/create.go	`37.14% <37.14%> (ø)`
cmd/client/commandv2/create/createhttpproxy.go	`87.23% <87.23%> (ø)`
cmd/builder/utils/context.go	`100.00% <100.00%> (ø)`
cmd/builder/utils/env.go	`100.00% <100.00%> (ø)`
cmd/builder/utils/validation.go	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

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

localvar · 2023-08-21T00:22:05Z

cmd/client/resources/customdatakind.go

+ var sb strings.Builder
+ sb.Grow(len(yamlBody))
+ for _, l := range lines {
+ if strings.HasPrefix(l, "name: ") {


Suggested change

if strings.HasPrefix(l, "name: ") {

if strings.HasPrefix(l, "name:") {

cmd/client/resources/object.go

localvar · 2023-08-28T01:31:47Z

README.md

+You can also create them using `egctl create httpproxy` command.
+```bash
+egctl create httpproxy demo --port 10080 \ 
+ --rule="/pipeline=http:https://127.0.0.1:9095,http:https://127.0.0.1:9096,http:https://127.0.0.1:9097"
+```
+this command will create `HTTPServer` `demo-server` and `Pipeline` `demo-pipeline-0` which work exactly same to `server-demo` and `pipeline-demo`. See more about [`egctl create httpproxy`](./doc/egctl-cheat-sheet.md#create-httpproxy).
+


propose not adding -server suffix to the name, user may get confused what he/she has created.
for the pipeline names, also propose not using the pipeline-x suffixes.

good idea. I will update that!

xxx7xxxx · 2023-08-30T03:37:42Z

cmd/client/commandv2/create/createhttpproxy.go

+ cmd.Flags().BoolVar(&o.TLS, "tls", false, "Enable TLS")
+ cmd.Flags().BoolVar(&o.AutoCert, "auto-cert", false, "Enable auto cert")
+ cmd.Flags().StringVar(&o.CaCertFile, "ca-cert-file", "", "CA cert file")
+ cmd.Flags().StringArrayVar(&o.CertFiles, "cert-file", []string{}, "Cert file")


Cert Files?

It use as --cert-file filename1 --cert-file filename2... If use --cert-files it feels more like --cert-files filename1,filename2,filename3, and it should use cmd.Flags().StringSliceVar. But it may cause some problem, since filename may itself contains ,.
Is this ok?

xxx7xxxx · 2023-08-30T03:37:52Z

cmd/client/commandv2/create/createhttpproxy.go

+ cmd.Flags().BoolVar(&o.AutoCert, "auto-cert", false, "Enable auto cert")
+ cmd.Flags().StringVar(&o.CaCertFile, "ca-cert-file", "", "CA cert file")
+ cmd.Flags().StringArrayVar(&o.CertFiles, "cert-file", []string{}, "Cert file")
+ cmd.Flags().StringArrayVar(&o.KeyFiles, "key-file", []string{}, "Key file")


same as --cert-file part...

xxx7xxxx · 2023-08-30T04:17:36Z

cmd/client/commandv2/logs.go

+ var n int
+ var follow bool
+ examples := []general.Example{
+ {Desc: "Print the most recent 500 logs by default.", Command: "egctl logs"},


Why not print all logs like other logs by default.

Yeah. Here are my concerns.

kubectl logs will print help information. If print logs kubectl logs <pod-name>. And actually egctl create, egctl apply will print help information. So, based on this, user may think egctl logs will print help information. They may think there are commands like egctl logs std or egctl logs http-access etc. So, by default, I choose egctl logs not print all logs, just a few.

easegress logs can be really really long. So print all logs may be not necessary...

Just my naive opinions, I am ok to change it to print all logs.

I don't know another command line to do like this, does this usage have a precedent?

Well, I am not sure these cases are precedent. But for really long logs, these tools not print them all by default.

git log only show a screen of logs, and allow scroll it.

journalctl, systemctl status is similar.

git log | cat will output all of them with cat to dismantle the pager effect. It just uses a pager to display logs, essentially it prints all of the logs. But egctl logs | cat outputs the last 500 lines. Well, I just provided another thought as I don't strongly insist on my point. You can decide on the design.

You are right. I am just a little afraid that in practice, when user run egctl logs, and terminal starts to print logs, then they press CTRL+C... (It happens to me all the time).
I am more prefer current design that provides a user-friendly default setting...

Then it is.

xxx7xxxx · 2023-08-30T04:19:05Z

cmd/client/commandv2/edit.go

+// EditCmd returns edit command.
+func EditCmd() *cobra.Command {
+ examples := []general.Example{
+ {Desc: "Edit a resource with name", Command: "egctl edit <resource> <name>"},


Could we specify editor like vim, nano.

Yes, in egctl-cheat-sheet.md, mentioned use of env variable of EGCTL_EDITOR to change editor. I will add an example about that in examples.

suchen-sci added 3 commits August 18, 2023 12:15

add egctl edit command

02a4901

add verbose flag for egctl describe

c527116

fix bug for edit custom data

e7cc5b7

suchen-sci requested review from localvar, xxx7xxxx and LokiWager August 18, 2023 09:36

update egctl cheat sheet

fad067a

localvar reviewed Aug 21, 2023

View reviewed changes

suchen-sci marked this pull request as draft August 21, 2023 04:27

suchen-sci added 2 commits August 23, 2023 19:13

finish egctl create httpproxy command, add some test

0782333

add more test for egctl create cmd

ff07cdb

suchen-sci changed the title ~~add egctl edit command~~ add egctl edit, egctl logs, update egctl create cmd Aug 24, 2023

suchen-sci added 5 commits August 24, 2023 16:25

add more test, add integration test

392a393

finish egctl logs command

267f0a0

optim egctl logs command

023dafe

add integration test for egctl logs

afcf76a

udpate doc, add examples

4b04d19

suchen-sci marked this pull request as ready for review August 25, 2023 10:04

suchen-sci added 2 commits August 25, 2023 18:11

update based on github action

0d9548f

close watcher

7f62308

localvar reviewed Aug 28, 2023

View reviewed changes

udpate based on comments

2ad235c

xxx7xxxx reviewed Aug 30, 2023

View reviewed changes

update egctl edit example

b38fc6b

xxx7xxxx approved these changes Aug 31, 2023

View reviewed changes

xxx7xxxx enabled auto-merge August 31, 2023 03:24

xxx7xxxx added this pull request to the merge queue Aug 31, 2023

Merged via the queue into easegress-io:main with commit 8f2dc59 Aug 31, 2023
7 checks passed

suchen-sci deleted the egctl-edit branch August 31, 2023 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add egctl edit, egctl logs, update egctl create cmd #1067

add egctl edit, egctl logs, update egctl create cmd #1067

suchen-sci commented Aug 18, 2023

codecov-commenter commented Aug 18, 2023 •

edited

localvar Aug 21, 2023

suchen-sci Aug 28, 2023

localvar Aug 28, 2023

suchen-sci Aug 28, 2023

xxx7xxxx Aug 30, 2023

suchen-sci Aug 30, 2023 •

edited

xxx7xxxx Aug 30, 2023

suchen-sci Aug 30, 2023

xxx7xxxx Aug 30, 2023

suchen-sci Aug 30, 2023

xxx7xxxx Aug 30, 2023

suchen-sci Aug 30, 2023

xxx7xxxx Aug 30, 2023

suchen-sci Aug 31, 2023

xxx7xxxx Aug 31, 2023

xxx7xxxx Aug 30, 2023

suchen-sci Aug 30, 2023

	if strings.HasPrefix(l, "name: ") {
	if strings.HasPrefix(l, "name:") {

add egctl edit, egctl logs, update egctl create cmd #1067

add egctl edit, egctl logs, update egctl create cmd #1067

Conversation

suchen-sci commented Aug 18, 2023

codecov-commenter commented Aug 18, 2023 • edited

Codecov Report

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

suchen-sci Aug 30, 2023 • edited

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

codecov-commenter commented Aug 18, 2023 •

edited

suchen-sci Aug 30, 2023 •

edited