Feature/helm pr deployment demo #9690
Conversation
Signed-off-by: Colin Wilk <colin.wilk@tum.de>
Bump versions for Chart and appVersion. Signed-off-by: Colin Wilk <colin.wilk@tum.de>
Add url for k8s env Signed-off-by: Colin Wilk <colin.wilk@tum.de>
Add condition to check if delpoy:k8s tag exists for the PR Signed-off-by: Colin Wilk <colin.wilk@tum.de>
Signed-off-by: Colin Wilk <colin.wilk@tum.de>
Signed-off-by: Colin Wilk <colin.wilk@tum.de>
Signed-off-by: Colin Wilk <colin.wilk@tum.de>
Signed-off-by: Colin Wilk <colin.wilk@tum.de>
Set cancel-in-progress to true so that in scenarios where commits are pushed fast or multiple tags are added at once we only run the most recent pipeline. Signed-off-by: Colin Wilk <colin.wilk@tum.de>
WalkthroughThe pull request introduces several updates across different files. The Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
helm/artemis/templates/NOTES.txt (1)
8-11: Consider adding a note about multi-node clustersThe current implementation gets the IP address of the first node, which might not be the correct node to access in a multi-node cluster. Consider adding a note to inform users they should use the IP address of the specific node they want to access.
export NODE_PORT=$(kubectl get --namespace {{ .Release.Namespace }} -o jsonpath="{.spec.ports[0].nodePort}" services {{ include "artemis.fullname" . }}) export NODE_IP=$(kubectl get nodes --namespace {{ .Release.Namespace }} -o jsonpath="{.items[0].status.addresses[0].address}") echo http://$NODE_IP:$NODE_PORT + echo "NOTE: In a multi-node cluster, you may need to use the IP address of the specific node you want to access"🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: Loose punctuation mark.
Context: ...ervices {{ include "artemis.fullname" . }}) export NODE_IP=$(kubectl get nodes ...(UNLIKELY_OPENING_PUNCTUATION)
helm/artemis/templates/_helpers.tpl (2)
33-43: Consider adding recommended Kubernetes label.The implementation follows Kubernetes labeling conventions. Consider adding the recommended
app.kubernetes.io/part-oflabel to better identify the application component.{{- define "artemis.labels" -}} helm.sh/chart: {{ include "artemis.chart" . }} {{ include "artemis.selectorLabels" . }} {{- if .Chart.AppVersion }} app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} {{- end }} app.kubernetes.io/managed-by: {{ .Release.Service }} +app.kubernetes.io/part-of: artemis {{- end }}
53-64: Remove unnecessary empty line.The implementation is correct, but there's an unnecessary empty line at the end of the function.
{{- define "artemis.serviceAccountName" -}} {{- if .Values.serviceAccount.create }} {{- default (include "artemis.fullname" .) .Values.serviceAccount.name }} {{- else }} {{- default "default" .Values.serviceAccount.name }} {{- end }} {{- end }} -
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (37)
.github/workflows/build.ymlis excluded by!**/*.yml.github/workflows/helm_pr_deployment.ymlis excluded by!**/*.yml.github/workflows/helm_pr_deployment_delete.ymlis excluded by!**/*.ymldocs/dev/testservers/github/k8s-deploy-complete.pngis excluded by!**/*.png,!**/*.pnghelm/artemis/Chart.lockis excluded by!**/*.lock,!**/*.lockhelm/artemis/Chart.yamlis excluded by!**/*.yamlhelm/artemis/templates/autoscaler/horizontal-pod-autoscaler.ymlis excluded by!**/*.ymlhelm/artemis/templates/configmaps/activemq-broker-configmap.ymlis excluded by!**/*.ymlhelm/artemis/templates/configmaps/artemis-ci-configmap.ymlis excluded by!**/*.ymlhelm/artemis/templates/configmaps/artemis-configmap.ymlis excluded by!**/*.ymlhelm/artemis/templates/configmaps/artemis-mysql-configmap.ymlis excluded by!**/*.ymlhelm/artemis/templates/configmaps/artemis-usermanagement-configmap.ymlis excluded by!**/*.ymlhelm/artemis/templates/configmaps/artemis-vcs-configmap.ymlis excluded by!**/*.ymlhelm/artemis/templates/configmaps/jhipster-registry-configmap.ymlis excluded by!**/*.ymlhelm/artemis/templates/configmaps/jhipster-registry-env-configmap.ymlis excluded by!**/*.ymlhelm/artemis/templates/deployments/_artemis-deployment.ymlis excluded by!**/*.ymlhelm/artemis/templates/deployments/activemq-broker.ymlis excluded by!**/*.ymlhelm/artemis/templates/deployments/artemis-deployment-profile.yamlis excluded by!**/*.yamlhelm/artemis/templates/deployments/artemis-deployment.yamlis excluded by!**/*.yamlhelm/artemis/templates/ingresses/ingress.yamlis excluded by!**/*.yamlhelm/artemis/templates/monitors/podmonitor.ymlis excluded by!**/*.ymlhelm/artemis/templates/pvc/artemis-mysql.ymlis excluded by!**/*.ymlhelm/artemis/templates/pvc/artemis.ymlis excluded by!**/*.ymlhelm/artemis/templates/secrets/activemq-broker-secrets.ymlis excluded by!**/*.ymlhelm/artemis/templates/secrets/artemis-secrets.ymlis excluded by!**/*.ymlhelm/artemis/templates/secrets/artemis-usermanagement-secrets.ymlis excluded by!**/*.ymlhelm/artemis/templates/secrets/jhipster-registry-secrets.ymlis excluded by!**/*.ymlhelm/artemis/templates/services/activemq-broker-service.ymlis excluded by!**/*.ymlhelm/artemis/templates/services/artemis-service-profile.yamlis excluded by!**/*.yamlhelm/artemis/templates/services/artemis-service.yamlis excluded by!**/*.yamlhelm/artemis/templates/services/jhipster-registry-service.yamlis excluded by!**/*.yamlhelm/artemis/templates/services/mysql-service.yamlis excluded by!**/*.yamlhelm/artemis/templates/statefulsets/artemis-mysql.ymlis excluded by!**/*.ymlhelm/artemis/templates/statefulsets/artemis-statefulset.yamlis excluded by!**/*.yamlhelm/artemis/templates/statefulsets/jhipster-registry.ymlis excluded by!**/*.ymlhelm/artemis/templates/tests/test-connection.yamlis excluded by!**/*.yamlhelm/artemis/values.yamlis excluded by!**/*.yaml
📒 Files selected for processing (4)
docs/dev/testservers.rst(2 hunks)helm/.gitignore(1 hunks)helm/artemis/templates/NOTES.txt(1 hunks)helm/artemis/templates/_helpers.tpl(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- helm/.gitignore
🧰 Additional context used
🪛 LanguageTool
helm/artemis/templates/NOTES.txt
[uncategorized] ~9-~9: Loose punctuation mark.
Context: ...ervices {{ include "artemis.fullname" . }}) export NODE_IP=$(kubectl get nodes ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~14-~14: Loose punctuation mark.
Context: ... svc -w {{ include "artemis.fullname" . }}' export SERVICE_IP=$(kubectl get svc...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~15-~15: Loose punctuation mark.
Context: ...pace }} {{ include "artemis.fullname" . }} --template "{{"{{ range (index .status...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~18-~18: Loose punctuation mark.
Context: ...tes.io/name={{ include "artemis.name" . }},app.kubernetes.io/instance={{ .Release...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (7)
helm/artemis/templates/NOTES.txt (1)
2-7: LGTM: Ingress configuration is well-implemented
The Ingress URL construction properly handles multiple hosts, paths, and TLS configuration.
helm/artemis/templates/_helpers.tpl (5)
1-6: LGTM! Follows Helm best practices for name generation.
The implementation correctly handles name overrides and follows DNS naming conventions with proper truncation.
8-24: LGTM! Comprehensive implementation of fully qualified name generation.
The function properly handles all naming scenarios and follows Kubernetes naming conventions.
26-31: LGTM! Proper chart label generation.
Correctly handles version string sanitization for Kubernetes label compatibility.
45-51: LGTM! Proper selector labels implementation.
Correctly implements the minimum required labels for Kubernetes selectors.
65-82: 🛠️ Refactor suggestion
Improve Spring profiles string generation.
The current implementation has several potential issues:
- No spaces after commas makes the profile string less readable
- Could generate invalid profile strings with double commas if values are empty
- No validation of provider values
Consider this improved implementation:
{{- define "artemis.springprofiles" -}}
-prod,artemis,decoupling
-{{- if .Values.application.userManagement.provider -}}
-,{{ .Values.application.userManagement.provider }}
-{{- end }}
-{{- if .Values.application.userManagement.ldap.enabled -}}
-,ldap
-{{- end }}
-{{- if .Values.application.versioncontrol.provider -}}
-,{{ .Values.application.versioncontrol.provider }}
-{{- end }}
-{{- if .Values.application.continuousintegration.provider -}}
-,{{ .Values.application.continuousintegration.provider }}
-{{- end }}
+{{- $profiles := list "prod" "artemis" "decoupling" -}}
+{{- if .Values.application.userManagement.provider -}}
+ {{- $profiles = append $profiles .Values.application.userManagement.provider -}}
+{{- end -}}
+{{- if .Values.application.userManagement.ldap.enabled -}}
+ {{- $profiles = append $profiles "ldap" -}}
+{{- end -}}
+{{- if .Values.application.versioncontrol.provider -}}
+ {{- $profiles = append $profiles .Values.application.versioncontrol.provider -}}
+{{- end -}}
+{{- if .Values.application.continuousintegration.provider -}}
+ {{- $profiles = append $profiles .Values.application.continuousintegration.provider -}}
+{{- end -}}
+{{- join "," $profiles -}}
{{- end }}This improved version:
- Uses Helm's list and join functions for better string generation
- Avoids potential double commas
- Is more maintainable and easier to extend
docs/dev/testservers.rst (1)
56-58: Verify the test server configuration details.
The new Kubernetes test server entry introduces some differences from other entries:
- The URL uses a wildcard pattern (
pr*) suggesting dynamic PR-based deployments - Admin accounts are set to "Always" instead of "On Demand" like other servers
Could you clarify:
- The reasoning behind having "Always" admin accounts?
- Whether there are any security implications of this configuration?
| {{- else if contains "ClusterIP" .Values.artemis.service.type }} | ||
| export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app.kubernetes.io/name={{ include "artemis.name" . }},app.kubernetes.io/instance={{ .Release.Name }}" -o jsonpath="{.items[0].metadata.name}") | ||
| export CONTAINER_PORT=$(kubectl get pod --namespace {{ .Release.Namespace }} $POD_NAME -o jsonpath="{.spec.containers[0].ports[0].containerPort}") | ||
| echo "Visit http://127.0.0.1:8080 to use your application" | ||
| kubectl --namespace {{ .Release.Namespace }} port-forward $POD_NAME 8080:$CONTAINER_PORT |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve ClusterIP configuration robustness
Two improvements are suggested:
- The local port (8080) should be configurable to avoid conflicts
- Pod selection should handle multiple replicas gracefully
export POD_NAME=$(kubectl get pods --namespace {{ .Release.Namespace }} -l "app.kubernetes.io/name={{ include "artemis.name" . }},app.kubernetes.io/instance={{ .Release.Name }}" -o jsonpath="{.items[0].metadata.name}")
export CONTAINER_PORT=$(kubectl get pod --namespace {{ .Release.Namespace }} $POD_NAME -o jsonpath="{.spec.containers[0].ports[0].containerPort}")
- echo "Visit http://127.0.0.1:8080 to use your application"
- kubectl --namespace {{ .Release.Namespace }} port-forward $POD_NAME 8080:$CONTAINER_PORT
+ export LOCAL_PORT={{ .Values.artemis.service.localPort | default "8080" }}
+ echo "Visit http://127.0.0.1:${LOCAL_PORT} to use your application"
+ echo "NOTE: If the application has multiple replicas, you might need to specify a different pod"
+ kubectl --namespace {{ .Release.Namespace }} port-forward $POD_NAME ${LOCAL_PORT}:$CONTAINER_PORTCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~18-~18: Loose punctuation mark.
Context: ...tes.io/name={{ include "artemis.name" . }},app.kubernetes.io/instance={{ .Release...
(UNLIKELY_OPENING_PUNCTUATION)
| {{- else if contains "LoadBalancer" .Values.artemis.service.type }} | ||
| NOTE: It may take a few minutes for the LoadBalancer IP to be available. | ||
| You can watch the status of by running 'kubectl get --namespace {{ .Release.Namespace }} svc -w {{ include "artemis.fullname" . }}' | ||
| export SERVICE_IP=$(kubectl get svc --namespace {{ .Release.Namespace }} {{ include "artemis.fullname" . }} --template "{{"{{ range (index .status.loadBalancer.ingress 0) }}{{.}}{{ end }}"}}") | ||
| echo http://$SERVICE_IP:{{ .Values.artemis.service.port }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance cloud provider compatibility for LoadBalancer IP retrieval
The current implementation might not work correctly with all cloud providers, as some return a hostname instead of an IP address in the LoadBalancer ingress field.
NOTE: It may take a few minutes for the LoadBalancer IP to be available.
You can watch the status of by running 'kubectl get --namespace {{ .Release.Namespace }} svc -w {{ include "artemis.fullname" . }}'
- export SERVICE_IP=$(kubectl get svc --namespace {{ .Release.Namespace }} {{ include "artemis.fullname" . }} --template "{{"{{ range (index .status.loadBalancer.ingress 0) }}{{.}}{{ end }}"}}")
+ export SERVICE_IP=$(kubectl get svc --namespace {{ .Release.Namespace }} {{ include "artemis.fullname" . }} --template "{{"{{ with index .status.loadBalancer.ingress 0 }}{{or .ip .hostname}}{{end}}")
echo http://$SERVICE_IP:{{ .Values.artemis.service.port }}
+ echo "NOTE: Some cloud providers may return a hostname instead of an IP address"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- else if contains "LoadBalancer" .Values.artemis.service.type }} | |
| NOTE: It may take a few minutes for the LoadBalancer IP to be available. | |
| You can watch the status of by running 'kubectl get --namespace {{ .Release.Namespace }} svc -w {{ include "artemis.fullname" . }}' | |
| export SERVICE_IP=$(kubectl get svc --namespace {{ .Release.Namespace }} {{ include "artemis.fullname" . }} --template "{{"{{ range (index .status.loadBalancer.ingress 0) }}{{.}}{{ end }}"}}") | |
| echo http://$SERVICE_IP:{{ .Values.artemis.service.port }} | |
| {{- else if contains "LoadBalancer" .Values.artemis.service.type }} | |
| NOTE: It may take a few minutes for the LoadBalancer IP to be available. | |
| You can watch the status of by running 'kubectl get --namespace {{ .Release.Namespace }} svc -w {{ include "artemis.fullname" . }}' | |
| export SERVICE_IP=$(kubectl get svc --namespace {{ .Release.Namespace }} {{ include "artemis.fullname" . }} --template "{{"{{ with index .status.loadBalancer.ingress 0 }}{{or .ip .hostname}}{{end}}") | |
| echo http://$SERVICE_IP:{{ .Values.artemis.service.port }} | |
| echo "NOTE: Some cloud providers may return a hostname instead of an IP address" |
🧰 Tools
🪛 LanguageTool
[uncategorized] ~14-~14: Loose punctuation mark.
Context: ... svc -w {{ include "artemis.fullname" . }}' export SERVICE_IP=$(kubectl get svc...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~15-~15: Loose punctuation mark.
Context: ...pace }} {{ include "artemis.fullname" . }} --template "{{"{{ range (index .status...
(UNLIKELY_OPENING_PUNCTUATION)
| GitHub Deployment | ||
| ----------------- | ||
|
|
||
| 1. Label your PR with deploy:k8s to initialize a deployment to Kubernetes. | ||
|
|
||
| 2. Once the server is deployed you get a message from GitHub Actions | ||
|
|
||
| .. figure:: testservers/github/k8s-deploy-complete.png | ||
| :alt: GitHub Actions Comment: successful deployment | ||
|
|
||
| 2. Perform your testing | ||
|
|
||
| a) For logs, log in to Rancher: https://rancher.ase.cit.tum.de | ||
|
|
||
| 3. As soon as you're done with this PR, close or merge it and the deployment will be destroyed. Don't remove the label! |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Documentation needs improvements for clarity and completeness.
The Kubernetes deployment instructions could be enhanced:
- Fix step numbering (step 2 appears twice)
- Add a screenshot showing where to find and apply the
deploy:k8slabel - Include expected deployment duration
- Add a troubleshooting section
- Explain what to do if deployment fails
Consider this structure:
GitHub Deployment
-----------------
1. Label your PR with deploy:k8s to initialize a deployment to Kubernetes.
+ .. figure:: testservers/github/k8s-label-location.png
+ :alt: GitHub UI: Location of deploy:k8s label
+
+ Note: Deployment typically takes 5-10 minutes.
2. Once the server is deployed you get a message from GitHub Actions
.. figure:: testservers/github/k8s-deploy-complete.png
:alt: GitHub Actions Comment: successful deployment
-2. Perform your testing
+3. Perform your testing
a) For logs, log in to Rancher: https://rancher.ase.cit.tum.de
-3. As soon as you're done with this PR, close or merge it and the deployment will be destroyed. Don't remove the label!
+4. As soon as you're done with this PR, close or merge it and the deployment will be destroyed. Don't remove the label!
+
+Troubleshooting
+---------------
+
+If deployment fails:
+1. Check the GitHub Actions logs for error messages
+2. Ensure your PR branch is up to date with the main branch
+3. Contact the DevOps team if issues persistCommittable suggestion skipped: line range outside the PR's diff.
Demo please ignore
Summary by CodeRabbit
Release Notes
New Features
NOTES.txtproviding users with access instructions based on service type.Chores
.gitignoreto exclude thechartsdirectory from version control.Refactor