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

[filter] refactor apiaggregator #153

Merged
merged 4 commits into from
Aug 3, 2021
Merged

[filter] refactor apiaggregator #153

merged 4 commits into from
Aug 3, 2021

Conversation

benja-wu
Copy link
Contributor

@benja-wu benja-wu commented Jul 27, 2021 •
edited
Loading

For closing #149

  1. Adding GetHTTPPipeline into RawConfigTrafficController
  2. Updating APIAggregator to use RawConfigTrafficController for getting pipeline in default namespace.

@benja-wu benja-wu added the enhancement New feature or request label Jul 27, 2021
@benja-wu benja-wu added this to the v1.2.0 milestone Jul 27, 2021
@benja-wu benja-wu requested a review from xxx7xxxx July 27, 2021 13:52
@benja-wu benja-wu linked an issue Jul 27, 2021 that may be closed by this pull request
The APIAggregator does not work #149
Closed
@benja-wu benja-wu requested a review from localvar July 27, 2021 13:53
megaeasex
megaeasex approved these changes Jul 27, 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 153 Deploy Test Success

@codecov-commenter
Copy link

codecov-commenter commented Jul 27, 2021 •
edited
Loading

Codecov Report

Merging #153 (947896f) into main (5df56c1) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #153      +/-   ##
==========================================
+ Coverage   57.81%   57.85%   +0.03%     
==========================================
  Files          38       37       -1     
  Lines        3658     3656       -2     
==========================================
  Hits         2115     2115              
+ Misses       1437     1435       -2     
  Partials      106      106
Impacted Files Coverage Δ
pkg/common/sys_proc_others.go

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 5df56c1...947896f. Read the comment docs.

localvar
localvar reviewed Jul 28, 2021
View reviewed changes
megaeasex
megaeasex suggested changes Jul 28, 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 FAILED]megaease/easegress Pull Request 153 Deploy Test failed

megaeasex
megaeasex suggested changes Jul 28, 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 FAILED]megaease/easegress Pull Request 153 Deploy Test failed

megaeasex
megaeasex approved these changes Jul 28, 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 153 Deploy Test Success

@haoel
Copy link
Contributor

haoel commented Jul 28, 2021

[TASK:easegress-pr-test FAILED]megaease/easegress Pull Request 153 Deploy Test failed

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

Schrödinger's bot!

megaeasex
megaeasex suggested changes Aug 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 FAILED]megaease/easegress Pull Request 153 Deploy Test failed

megaeasex
megaeasex approved these changes Aug 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 153 Deploy Test Success

xxx7xxxx
xxx7xxxx reviewed Aug 3, 2021
View reviewed changes
pkg/filter/apiaggregator/apiaggregator.go
PartialSucceed bool `yaml:"partialSucceed"`
Timeout string `yaml:"timeout" jsonschema:"omitempty,format=duration"`
MergeResponse bool `yaml:"mergeResponse"`
MaxBodyBytes int64 `yaml:"maxBodyBytes" jsonschema:"omitempty,minimum=0,maximum=102400"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Prose add Namespace in spec with default value default, in case of another component use this filter to aggregate apis.

Copy link
Contributor Author

@benja-wu benja-wu Aug 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

If we provide this namespace field, that means I can create a Pipeline with several aggregators to represent different namespace's Pipelines. That's too powerful as just being a filter. (be able to help one pipeline across different namespaces).
So I prefer to keep this filter be used only in the default namespace currently.

@xxx7xxxx xxx7xxxx merged commit c32e42c into easegress-io:main Aug 3, 2021
@benja-wu benja-wu deleted the aggregator branch August 12, 2021 05:26
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

@xxx7xxxx xxx7xxxx xxx7xxxx approved these changes

@megaeasex megaeasex megaeasex approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.

The APIAggregator does not work
6 participants
@benja-wu @codecov-commenter @haoel @localvar @xxx7xxxx @megaeasex