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.

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

Remove unused helm values #1277

Merged
merged 1 commit into from
Apr 11, 2023
Merged

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Apr 10, 2023

No description provided.

@arkodg arkodg requested a review from a team as a code owner April 10, 2023 17:44
Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg arkodg force-pushed the unusued-helm-values branch from f340649 to a2a8f07 Compare April 10, 2023 17:45
@arkodg
Copy link
Contributor Author

arkodg commented Apr 10, 2023

hey @Xunzhuo should we also disable metrics-service and kube-rbac-proxy for 0.4.0 and enable it later in a future release ?

@codecov
Copy link

codecov bot commented Apr 10, 2023

Codecov Report

Merging #1277 (a2a8f07) into main (78b62e4) will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1277      +/-   ##
==========================================
- Coverage   61.10%   61.06%   -0.05%     
==========================================
  Files          85       85              
  Lines       10766    10766              
==========================================
- Hits         6579     6574       -5     
- Misses       3756     3760       +4     
- Partials      431      432       +1     

see 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@@ -69,7 +69,7 @@ spec:
- --v=0
env:
- name: KUBERNETES_CLUSTER_DOMAIN
value: {{ .Values.kubernetesClusterDomain }}
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Imo we need to treat helm values as APi fields and expose them when needed, else we cause breaking changes, the goal of this PR is to expose minimum helm values needed for 0.4.0

Copy link
Contributor

Choose a reason for hiding this comment

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

but this need be configurable in the future, xref: #1280

@Xunzhuo
Copy link
Member

Xunzhuo commented Apr 11, 2023

Thoughts on #1277 (comment) ?
cc @arkodg, we can remove it for now

@Xunzhuo Xunzhuo merged commit bb014eb into envoyproxy:main Apr 11, 2023
@Xunzhuo
Copy link
Member

Xunzhuo commented Apr 11, 2023

Merge it since I am adding docs for values

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants