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

Add a Helm Chart for Flow Aggregator #3952

Merged
merged 1 commit into from
Jul 1, 2022
Merged

Add a Helm Chart for Flow Aggregator #3952

merged 1 commit into from
Jul 1, 2022

Conversation

yanjunz97
Copy link
Contributor

As Antrea has introduced Helm support, this PR takes care of the Helm support for Flow Aggregator by

  • Adding a Flow Aggregator Helm Chart
  • Using the Helm templates and Kustomize (for injecting namespace) to generate the Flow Aggregator manifest
  • Supporting to generate Helm chart archive and upload it as a release asset for Antrea release

Signed-off-by: Yanjun Zhou [email protected]

@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2022 •
edited
Loading

Codecov Report

Merging #3952 (0c4a3cb) into main (863458b) will increase coverage by 0.68%.
The diff coverage is 44.44%.

❗ Current head 0c4a3cb differs from pull request most recent head 7eb9342. Consider uploading reports for the commit 7eb9342 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3952      +/-   ##
==========================================
+ Coverage   64.29%   64.97%   +0.68%     
==========================================
  Files         295      295              
  Lines       43672    43672              
==========================================
+ Hits        28077    28377     +300     
+ Misses      13342    13035     -307     
- Partials     2253     2260       +7
Flag Coverage Δ *Carryforward flag
e2e-tests 41.14% <ø> (?)
kind-e2e-tests 51.20% <ø> (+0.54%) ⬆️ Carriedforward from 48c0677
unit-tests 44.42% <44.44%> (+0.06%) ⬆️ Carriedforward from 48c0677

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
...icluster/cmd/multicluster-controller/controller.go 6.45% <0.00%> (ø)
multicluster/cmd/multicluster-controller/leader.go 0.00% <ø> (ø)
...llers/multicluster/member_clusterset_controller.go 9.73% <0.00%> (ø)
...ntrollers/multicluster/serviceexport_controller.go 66.47% <ø> (ø)
...llers/multicluster/leader_clusterset_controller.go 63.76% <66.66%> (ø)
...uster/controllers/multicluster/controller_utils.go 27.55% <83.33%> (ø)
...lticluster/commonarea/resourceimport_controller.go 68.90% <100.00%> (ø)
pkg/agent/nodeportlocal/k8s/annotations.go 84.44% <0.00%> (-15.56%) ⬇️
pkg/apiserver/handlers/endpoint/handler.go 56.52% <0.00%> (-13.05%) ⬇️
...agent/flowexporter/connections/deny_connections.go 72.41% <0.00%> (-11.50%) ⬇️
... and 44 more

antoninbas
antoninbas reviewed Jun 30, 2022
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

minor comments, LGTM

build/charts/flow-aggregator/Chart.yaml Outdated Show resolved Hide resolved
hack/generate-helm-release.sh Outdated Show resolved Hide resolved
hack/generate-helm-release.sh Outdated Show resolved Hide resolved
hack/generate-manifest-flow-aggregator.sh Outdated Show resolved Hide resolved
build/yamls/flow-aggregator/base/namespace.yaml Outdated Show resolved Hide resolved
build/charts/flow-aggregator/values.yaml Outdated Show resolved Hide resolved
build/charts/flow-aggregator/values.yaml Outdated Show resolved Hide resolved
@yanjunz97
Copy link
Contributor Author

/test-all

dreamtalen
dreamtalen reviewed Jun 30, 2022
View reviewed changes
Copy link
Contributor

@dreamtalen dreamtalen left a comment

Choose a reason for hiding this comment

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

A nit only, LGTM

hack/generate-manifest-flow-aggregator.sh Outdated Show resolved Hide resolved
@yanjunz97 yanjunz97 force-pushed the fa-helm branch 2 times, most recently from 727e735 to 71cf342 Compare June 30, 2022 21:27
@yanjunz97
Copy link
Contributor Author

/test-all

yuntanghsu
yuntanghsu reviewed Jun 30, 2022
View reviewed changes
build/yamls/flow-aggregator.yml
@@ -323,8 +322,6 @@ spec:
secretKeyRef:
key: password
name: clickhouse-secret
- name: FA_CONFIG_MAP_NAME
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should keep FA_CONFIG_MAP_NAME? I used this to have consistency with antrea.
https://github.com/antrea-io/antrea/blob/main/build/yamls/antrea.yml#L3983-L3984

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically I think it is not necessary to save this in the environment variable as the config map name will be constant, but I do not have strong preference.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think it is fine either way
But it doesn't hurt to be consistent with Antrea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Changed it back to the consistent version.

@yanjunz97
Copy link
Contributor Author

/test-all

antoninbas
antoninbas approved these changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

@antoninbas antoninbas merged commit 79b6480 into antrea-io:main Jul 1, 2022
@antoninbas antoninbas added action/release-note Indicates a PR that should be included in release notes. area/build-release Issues or PRs related to building and releasing labels Jul 1, 2022
@yanjunz97 yanjunz97 mentioned this pull request Jul 1, 2022
Merged
yanjunz97 added a commit to antrea-io/theia that referenced this pull request Jul 15, 2022
@yanjunz97
Remove the Flow Aggregator Helm Chart (#70)
51fb83e 
Flow Aggregator Helm Chart is added to Antrea repo with PR antrea-io/antrea#3952. This PR removes it from Theia repo.
Signed-off-by: Yanjun Zhou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreamtalen dreamtalen dreamtalen left review comments

@yuntanghsu yuntanghsu yuntanghsu left review comments

@antoninbas antoninbas antoninbas approved these changes

@heanlan heanlan Awaiting requested review from heanlan

@salv-orlando salv-orlando Awaiting requested review from salv-orlando

Assignees
No one assigned
Labels
action/release-note Indicates a PR that should be included in release notes. area/build-release Issues or PRs related to building and releasing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yanjunz97 @codecov-commenter @antoninbas @dreamtalen @yuntanghsu