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

[Chart] frontend service annotation #587

Merged
merged 2 commits into from
Sep 6, 2018

Conversation

migmartri
Copy link
Contributor

@migmartri migmartri commented Sep 6, 2018

It sdds support for metadata annotations to the frontend service.

This is useful if the user opts for exposing kubeapps via a LoadBalancer but want to tweak it by making it for example internal 'service.beta.kubernetes.io/aws-load-balancer-internal: 0.0.0.0/0'

I have not modified the Readme because in my opinion, this is a power user feature, disabled by default, and not commonly required.

@migmartri migmartri changed the title Adds support for metadata annotations to the frontend service. [Chart] frontend service annotation Sep 6, 2018
Copy link
Contributor

@prydonius prydonius left a comment

Choose a reason for hiding this comment

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

lgtm

@migmartri migmartri merged commit 830040f into vmware-tanzu:master Sep 6, 2018
@migmartri migmartri deleted the annotation-ingress branch September 6, 2018 00:26
@@ -92,6 +92,7 @@ frontend:
service:
port: 80
type: ClusterIP
# annotations: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why commenting it out? if it is empty, it doesn't need to be commented out. And the rest of optional values (like tolerations) are not commented out. Is this a special case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching the inconsistency, no, it is not a special case, I think I just left it like that by mistake because I was testing the differences between range used upstream https://github.com/helm/charts/blob/master/stable/wordpress/templates/ingress.yaml#L16 and with values that we use in this chart.

My bad, sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it seems that it is consistent with the way we use annotations in the ingress.

https://github.com/kubeapps/kubeapps/blob/1af290624f2fb6e0ca1d5972a665d2d389d69888/chart/kubeapps/values.yaml#L143-L145

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually commented out vs empty object in the values.yaml file does not seem to make any difference, so I am wondering why we commented it out in the ingress in the first place Maybe @prydonius could shed some light?

With the current commented out value, no annotation added

helm template . -x templates/kubeapps-frontend-service.yaml 
---
# Source: kubeapps/templates/kubeapps-frontend-service.yaml
apiVersion: v1
kind: Service
metadata:
  name: RELEASE-NAME-kubeapps
  labels:
    app: kubeapps
    chart: kubeapps-0.4.0
    release: RELEASE-NAME
    heritage: Tiller
spec:

Using the empty object approach instead, no annotation added either

git diff
diff --git a/chart/kubeapps/values.yaml b/chart/kubeapps/values.yaml
index fa97086c..7b65f76c 100644
--- a/chart/kubeapps/values.yaml
+++ b/chart/kubeapps/values.yaml
@@ -92,7 +92,7 @@ frontend:
   service:
     port: 80
     type: ClusterIP
-    # annotations: {}
+    annotations: {}
   livenessProbe:

 helm template . -x templates/kubeapps-frontend-service.yaml 
---
# Source: kubeapps/templates/kubeapps-frontend-service.yaml
apiVersion: v1
kind: Service
metadata:
  name: RELEASE-NAME-kubeapps
  labels:
    app: kubeapps
    chart: kubeapps-0.4.0
    release: RELEASE-NAME
    heritage: Tiller
spec:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In case that we decide to change it, I went ahead and created a patch for it #599

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