bito-code-review[bot] commented on code in PR #39350: URL: https://github.com/apache/superset/pull/39350#discussion_r3466860424
########## helm/superset/tests/labels_test.yaml: ########## @@ -0,0 +1,386 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +suite: Label Consistency Tests +templates: + - deployment.yaml + - deployment-worker.yaml + - deployment-beat.yaml + - deployment-flower.yaml + - deployment-ws.yaml + - service.yaml + - service-ws.yaml + - service-flower.yaml + - init-job.yaml + - ingress.yaml + +# These tests validate that Kubernetes recommended labels are consistently applied +# across all chart resources per https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ +# +# Required Labels (app.kubernetes.io/): +# - name: The name of the application +# - instance: A unique name identifying the instance of an application +# - version: The current version of the application +# - component: The component within the architecture +# - part-of: The name of a higher level application this one is part of +# - managed-by: The tool being used to manage the operation of an application +# +# Helm-specific Labels: +# - helm.sh/chart: The chart name and version + +tests: + # ============================================================================= + # Main Deployment Labels + # ============================================================================= + - it: should have all recommended labels on main deployment + template: deployment.yaml + asserts: + - isNotNull: + path: metadata.labels["app.kubernetes.io/name"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/instance"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/version"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/managed-by"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/part-of"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/component"] + - isNotNull: + path: metadata.labels["helm.sh/chart"] + + - it: should have correct component label on main deployment + template: deployment.yaml + asserts: + - equal: + path: metadata.labels["app.kubernetes.io/component"] + value: web + + - it: should have part-of label set to superset on main deployment + template: deployment.yaml + asserts: + - equal: + path: metadata.labels["app.kubernetes.io/part-of"] + value: superset + + # ============================================================================= + # Worker Deployment Labels + # ============================================================================= + - it: should have all recommended labels on worker deployment + template: deployment-worker.yaml + asserts: + - isNotNull: + path: metadata.labels["app.kubernetes.io/name"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/instance"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/version"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/managed-by"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/part-of"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/component"] + + - it: should have correct component label on worker deployment + template: deployment-worker.yaml + asserts: + - equal: + path: metadata.labels["app.kubernetes.io/component"] + value: worker + + # ============================================================================= + # Celery Beat Deployment Labels + # ============================================================================= + - it: should have all recommended labels on celerybeat deployment + template: deployment-beat.yaml + set: + supersetCeleryBeat.enabled: true + asserts: + - isNotNull: + path: metadata.labels["app.kubernetes.io/name"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/instance"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/component"] + + - it: should have correct component label on celerybeat deployment + template: deployment-beat.yaml + set: + supersetCeleryBeat.enabled: true + asserts: + - equal: + path: metadata.labels["app.kubernetes.io/component"] + value: celerybeat + + # ============================================================================= + # Flower Deployment Labels + # ============================================================================= + - it: should have all recommended labels on flower deployment + template: deployment-flower.yaml + set: + supersetCeleryFlower.enabled: true + asserts: + - isNotNull: + path: metadata.labels["app.kubernetes.io/name"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/instance"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/component"] + + - it: should have correct component label on flower deployment + template: deployment-flower.yaml + set: + supersetCeleryFlower.enabled: true + asserts: + - equal: + path: metadata.labels["app.kubernetes.io/component"] + value: flower + + # ============================================================================= + # WebSocket Deployment Labels + # ============================================================================= + - it: should have all recommended labels on websocket deployment + template: deployment-ws.yaml + set: + supersetWebsockets.enabled: true + supersetWebsockets.config.jwtSecret: "test-secret-for-unit-test" + asserts: + - isNotNull: + path: metadata.labels["app.kubernetes.io/name"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/instance"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/component"] + + - it: should have correct component label on websocket deployment + template: deployment-ws.yaml + set: + supersetWebsockets.enabled: true + supersetWebsockets.config.jwtSecret: "test-secret-for-unit-test" + asserts: + - equal: + path: metadata.labels["app.kubernetes.io/component"] + value: websocket + + # ============================================================================= + # Service Labels + # ============================================================================= + - it: should have all recommended labels on main service + template: service.yaml + asserts: + - isNotNull: + path: metadata.labels["app.kubernetes.io/name"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/instance"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/component"] + + - it: should have correct component label on main service + template: service.yaml + asserts: + - equal: + path: metadata.labels["app.kubernetes.io/component"] + value: web + + - it: should have all recommended labels on websocket service + template: service-ws.yaml + set: + supersetWebsockets.enabled: true + supersetWebsockets.config.jwtSecret: "test-secret-for-unit-test" + asserts: + - isNotNull: + path: metadata.labels["app.kubernetes.io/name"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/instance"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/component"] + + - it: should have correct component label on websocket service + template: service-ws.yaml + set: + supersetWebsockets.enabled: true + supersetWebsockets.config.jwtSecret: "test-secret-for-unit-test" + asserts: + - equal: + path: metadata.labels["app.kubernetes.io/component"] + value: websocket + + - it: should have all recommended labels on flower service + template: service-flower.yaml + set: + supersetCeleryFlower.enabled: true + asserts: + - isNotNull: + path: metadata.labels["app.kubernetes.io/name"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/instance"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/component"] + + - it: should have correct component label on flower service + template: service-flower.yaml + set: + supersetCeleryFlower.enabled: true + asserts: + - equal: + path: metadata.labels["app.kubernetes.io/component"] + value: flower + + # ============================================================================= + # Init Job Labels + # ============================================================================= + - it: should have all recommended labels on init job + template: init-job.yaml + set: + init.enabled: true + init.createAdmin: true + init.adminUser.password: "test-password" + asserts: + - isNotNull: + path: metadata.labels["app.kubernetes.io/name"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/instance"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/component"] + + - it: should have correct component label on init job + template: init-job.yaml + set: + init.enabled: true + init.createAdmin: true + init.adminUser.password: "test-password" + asserts: + - equal: + path: metadata.labels["app.kubernetes.io/component"] + value: init + + # ============================================================================= + # Ingress Labels + # ============================================================================= + - it: should have all recommended labels on ingress + template: ingress.yaml + set: + ingress.enabled: true + ingress.hosts: + - host: superset.example.com + paths: + - path: / + pathType: Prefix + asserts: + - isNotNull: + path: metadata.labels["app.kubernetes.io/name"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/instance"] + - isNotNull: + path: metadata.labels["app.kubernetes.io/component"] + + - it: should have correct component label on ingress + template: ingress.yaml + set: + ingress.enabled: true + ingress.hosts: + - host: superset.example.com + paths: + - path: / + pathType: Prefix + asserts: + - equal: + path: metadata.labels["app.kubernetes.io/component"] + value: ingress + + # ============================================================================= + # Selector Label Consistency + # + # These use value assertions (not isNotNull) on purpose: a missing/misscoped + # release name renders as the string "<no value>", which is non-null and would + # silently pass isNotNull. Asserting the concrete value catches that class of + # bug, and asserting the pod template labels equal the selector guards the + # immutable spec.selector.matchLabels <-> pod label invariant. + # ============================================================================= + - it: should set selector matchLabels to concrete values on main deployment + template: deployment.yaml + asserts: + - equal: + path: spec.selector.matchLabels["app.kubernetes.io/name"] + value: superset + - equal: + path: spec.selector.matchLabels["app.kubernetes.io/instance"] + value: RELEASE-NAME + - equal: + path: spec.selector.matchLabels["app.kubernetes.io/component"] + value: web + + - it: should match pod template labels to the selector on main deployment + template: deployment.yaml + asserts: + - equal: + path: spec.template.metadata.labels["app.kubernetes.io/name"] + value: superset + - equal: + path: spec.template.metadata.labels["app.kubernetes.io/instance"] + value: RELEASE-NAME + - equal: + path: spec.template.metadata.labels["app.kubernetes.io/component"] + value: web + + - it: should set selector matchLabels to concrete values on worker deployment + template: deployment-worker.yaml + asserts: + - equal: + path: spec.selector.matchLabels["app.kubernetes.io/instance"] + value: RELEASE-NAME + - equal: + path: spec.selector.matchLabels["app.kubernetes.io/component"] + value: worker + + - it: should match pod template labels to the selector on worker deployment + template: deployment-worker.yaml + asserts: + - equal: + path: spec.template.metadata.labels["app.kubernetes.io/instance"] + value: RELEASE-NAME + - equal: + path: spec.template.metadata.labels["app.kubernetes.io/component"] + value: worker + + # ============================================================================= + # Extra Labels Support + # ============================================================================= + - it: should include extraLabels when specified + template: deployment.yaml + set: + extraLabels: + custom-label: custom-value + environment: production + asserts: + - equal: + path: metadata.labels.custom-label + value: custom-value + - equal: + path: metadata.labels.environment + value: production + + - it: should include extraLabels in service + template: service.yaml + set: + extraLabels: + custom-label: custom-value + asserts: + - equal: + path: metadata.labels.custom-label + value: custom-value Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Tests target unimplemented feature</b></div> <div id="fix"> Tests at lines 364-386 assert that `extraLabels` appear in `deployment.yaml` and `service.yaml` metadata, but neither template includes `extraLabels` in its labels section. The `deployment.yaml` labels block (lines 25-29) only renders `superset.componentLabels` and `supersetNode.deploymentLabels`. The `service.yaml` labels block (lines 25-26) only renders `superset.componentLabels`. Compare with `pdb.yaml` (lines 34-35) which correctly includes `{{- if $.Values.extraLabels }}{{- toYaml $.Values.extraLabels | nindent 4 }}{{- end }}`. These tests will fail at runtime. </div> </div> <small><i>Code Review Run #15db68</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## helm/superset/templates/_helpers.tpl: ########## @@ -61,6 +61,49 @@ Create chart name and version as used by the chart label. {{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" -}} {{- end -}} +{{/* +Common labels for all resources - follows Kubernetes recommended labels +https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ +*/}} +{{- define "superset.labels" -}} +helm.sh/chart: {{ include "superset.chart" . }} +{{ include "superset.selectorLabels" . }} +{{- if .Chart.AppVersion }} +app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} +{{- end }} +app.kubernetes.io/managed-by: {{ .Release.Service }} +app.kubernetes.io/part-of: superset +{{- if .Values.extraLabels }} +{{ toYaml .Values.extraLabels }} +{{- end }} +{{- end -}} Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Inconsistent label format across templates</b></div> <div id="fix"> The diff introduces Kubernetes recommended labels in `_helpers.tpl` (lines 64-79), but 8 other template files still use the old label format (`app:`, `chart:`, `release:`, `heritage:`). This creates inconsistent labeling across the chart. Files affected: `configmap-superset.yaml` (lines 27,29), `pdb.yaml` (lines 30,32), `pdb-beat.yaml` (lines 30,32), `pdb-flower.yaml` (lines 30,32), `pdb-worker.yaml` (lines 30,32), `pdb-ws.yaml` (lines 30,32), `secret-superset-config.yaml` (lines 26,28), `secret-ws.yaml` (lines 27,29). </div> </div> <small><i>Code Review Run #15db68</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
