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.

Sign up for GitHub

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

Jump to bottom

improve egctl to support sending multi configs at once (fix #29) #230

Merged
merged 5 commits into from
Sep 7, 2021
Merged

improve egctl to support sending multi configs at once (fix #29) #230

merged 5 commits into from
Sep 7, 2021

Conversation

nevill
Copy link
Contributor

@nevill nevill commented Sep 2, 2021

I did a fix on #29 . It sends multiple requests via egctl, based on how many YAML parts (separated by ---) it read.

I know it's quick and dirty 😌, but if possible, someone can lead me to work more on it.

megaeasex
megaeasex approved these changes Sep 2, 2021
View reviewed changes
Copy link

@megaeasex megaeasex left a comment

Choose a reason for hiding this comment

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

[TASK:easegress-pr-test SUCCESS]megaease/easegress Pull Request 230 Deploy Test Success

@benja-wu benja-wu added the enhancement New feature or request label Sep 2, 2021
@benja-wu benja-wu added this to In progress in Easegress Project via automation Sep 2, 2021
@benja-wu benja-wu linked an issue Sep 2, 2021 that may be closed by this pull request
Group configurations by a single Yaml file #29
Closed
@benja-wu
Copy link
Contributor

benja-wu commented Sep 2, 2021 •
edited

Hi @nevill

  • First of all, thanks for your contribution.

  • Currently, this quick fixing is acceptable to me personally. But in the future refactoring, I think we should align this egctl to kubectl or emctl(CTL from our mesh product). They are all using the visitor pattern[1]. That will be cooler than our current implementation.

  • If u are also interesting in the emctl's visitor implementation, here is the source code. Then we can wait for your refactoring this egctl with the visitor pattern in this PR too.

Reference

  1. https://en.wikipedia.org/wiki/Visitor_pattern

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2021 •
edited

Codecov Report

Merging #230 (5897eba) into main (e1d0a82) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #230   +/-   ##
=======================================
  Coverage   80.12%   80.12%           
=======================================
  Files          44       44           
  Lines        4835     4835           
=======================================
  Hits         3874     3874           
  Misses        740      740           
  Partials      221      221

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aad887f...5897eba. Read the comment docs.

@nevill
Copy link
Contributor Author

nevill commented Sep 2, 2021

Thanks @benja-wu for the info. Let me take a look at emctl source first.

@benja-wu
Copy link
Contributor

benja-wu commented Sep 2, 2021 •
edited

Thanks @benja-wu for the info. Let me take a look at emctl source first.

And one more thing, the code analysis format checking is failed.
Please make sure such formatting execution is successfully in your local env before contributing a PR, thx.

localvar
localvar requested changes Sep 3, 2021
View reviewed changes
cmd/client/command/common.go Outdated Show resolved Hide resolved
cmd/client/command/common.go Outdated Show resolved Hide resolved
Easegress Project automation moved this from In progress to Review in progress Sep 3, 2021
megaeasex
megaeasex approved these changes Sep 3, 2021
View reviewed changes
Copy link

@megaeasex megaeasex left a comment

Choose a reason for hiding this comment

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

[TASK:easegress-pr-test SUCCESS]megaease/easegress Pull Request 230 Deploy Test Success

megaeasex
megaeasex approved these changes Sep 6, 2021
View reviewed changes
Copy link

@megaeasex megaeasex left a comment

Choose a reason for hiding this comment

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

[TASK:easegress-pr-test SUCCESS]megaease/easegress Pull Request 230 Deploy Test Success

@nevill
Copy link
Contributor Author

nevill commented Sep 6, 2021

@benja-wu PR is updated. A new visitor for YAML parsing is added. Also @localvar 's advice is taken, to validate all the docs before sending requests to server.

localvar
localvar reviewed Sep 7, 2021
View reviewed changes
cmd/client/command/visitor.go Outdated Show resolved Hide resolved
cmd/client/command/common.go Outdated Show resolved Hide resolved
cmd/client/command/visitor.go Outdated Show resolved Hide resolved
@nevill @localvar
Update comments.
e3b448e 
Co-authored-by: Bomin Zhang <[email protected]>
megaeasex
megaeasex approved these changes Sep 7, 2021
View reviewed changes
Copy link

@megaeasex megaeasex left a comment

Choose a reason for hiding this comment

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

[TASK:easegress-pr-test SUCCESS]megaease/easegress Pull Request 230 Deploy Test Success

@nevill
some improvements
5897eba 
- change the signature of VisitorFunc
- remove useless function readFromFileOrStdin
megaeasex
megaeasex approved these changes Sep 7, 2021
View reviewed changes
Copy link

@megaeasex megaeasex left a comment

Choose a reason for hiding this comment

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

[TASK:easegress-pr-test SUCCESS]megaease/easegress Pull Request 230 Deploy Test Success

@nevill nevill requested a review from localvar September 7, 2021 08:56
Easegress Project automation moved this from Review in progress to Reviewer approved Sep 7, 2021
benja-wu
benja-wu approved these changes Sep 7, 2021
View reviewed changes
Copy link
Contributor

@benja-wu benja-wu left a comment

Choose a reason for hiding this comment

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

LGTM

@benja-wu benja-wu merged commit 4749392 into easegress-io:main Sep 7, 2021
Easegress Project automation moved this from Reviewer approved to Done Sep 7, 2021
@nevill nevill deleted the support-multi-config-in-one branch September 8, 2021 00:49
@benja-wu benja-wu mentioned this pull request Sep 11, 2021
Merged
xmh19936688 pushed a commit to xmh19936688/easegress that referenced this pull request Sep 13, 2021
@nevill @localvar @xmh19936688
improve egctl to support sending multi configs at once (fix #29) (eas…
71cbf1e 
…egress-io#230)

* improve egctl to support sending multi configs at once (fix #29)

* add a visitor to do the YAML parsing

* rollback function readFromFileOrStdin

* Update comments.

Co-authored-by: Bomin Zhang <[email protected]>

* some improvements
- change the signature of VisitorFunc
- remove useless function readFromFileOrStdin

Co-authored-by: Bomin Zhang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@localvar localvar localvar approved these changes

@benja-wu benja-wu benja-wu approved these changes

@megaeasex megaeasex megaeasex approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
Easegress Project
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Group configurations by a single Yaml file
5 participants
@nevill @benja-wu @codecov-commenter @localvar @megaeasex