Feat/k8s nats helm #225
Feat/k8s nats helm #225
Conversation
* Refactor job queue handling to support priority-based processing and update related tests * Enhance job stream configuration to disallow duplicates * Add KeyValue store for job management * Add Bruno files for NATS
Switch from Redis to NATS (#2)
…#219) * update bruno script to show failure if it fails * add shared volumes to fix "folder not found error" * ensure debug mode logs deletion of volume * implement delete volume after creation before execution flag
## Walkthrough
This update introduces a new Helm chart for deploying the Hades CI/CD system on Kubernetes. It adds configuration files, deployment and service templates for both the API and scheduler components, RBAC resources, ingress configuration, and supporting files like `.gitignore`, `.helmignore`, and documentation. Additionally, a typo is corrected in an API documentation script. The Kubernetes client initialization is enhanced to support in-cluster configuration for the scheduler.
## Changes
| File(s) | Change Summary |
|-------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------|
| docs/environments/Kubernetes.bru | Added a configuration file defining a `hostname` variable for Kubernetes environments. |
| helm/hades/.gitignore, helm/hades/.helmignore | Added ignore files for Helm chart packaging and Git version control. |
| helm/hades/Chart.yaml | Introduced Helm chart metadata for the "hades" application, including dependencies and versioning. |
| helm/hades/values.yaml | Added chart configuration values for deployment, services, RBAC, ingress, and NATS messaging. |
| helm/hades/templates/hades-api-deployment.yaml, helm/hades/templates/hades-api-service.yaml | Added Kubernetes Deployment and Service templates for the Hades API component. |
| helm/hades/templates/hades-scheduler-deployment.yaml, helm/hades/templates/hades-scheduler-service.yaml | Added Deployment and Service templates for the Hades Scheduler component. |
| helm/hades/templates/hades-scheduler-role.yaml, helm/hades/templates/hades-scheduler-rolebinding.yaml, helm/hades/templates/hades-scheduler-serviceaccount.yaml | Added RBAC Role, RoleBinding, and ServiceAccount templates for the scheduler. |
| helm/hades/templates/ingress.yaml | Added conditional Ingress template for API exposure with TLS and annotations support. |
| helm/hades/readme.md | Added README documenting Helm chart usage, configuration, and deployment steps. |
| docs/api/Create Build Job (Test Fail).bru | Fixed a typo in a shell script command in the API documentation. |
| HadesScheduler/k8s/clusterAccess.go | Added `initializeInCluster()` function to create Kubernetes clientset using in-cluster configuration. |
| HadesScheduler/k8s/k8s.go | Modified `initializeClusterAccess` to support service account mode by using in-cluster clientset initialization. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant Helm
participant Kubernetes API
participant Hades API Pod
participant Hades Scheduler Pod
participant NATS
User->>Helm: Install hades chart with values.yaml
Helm->>Kubernetes API: Apply manifests (Deployments, Services, RBAC, Ingress)
Kubernetes API->>Hades API Pod: Start pod
Kubernetes API->>Hades Scheduler Pod: Start pod
Hades API Pod->>NATS: Connect using configured host/port
Hades Scheduler Pod->>NATS: Connect using configured host/port
User->>Ingress: Access /build endpoint
Ingress->>Hades API Pod: Forward requestEstimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
shared/utils/queue.go (1)
242-242: Add error handling for message acknowledgment failures.The
msg.Nak()calls should check for errors to improve observability of acknowledgment failures, as flagged in previous reviews.Apply this pattern to all
msg.Nak()calls:if err != nil { slog.Error("Failed to parse message ID", "error", err, "data", string(msg.Data())) - msg.Nak() // Negative acknowledgment, message will be redelivered + if nakErr := msg.Nak(); nakErr != nil { + slog.Error("Failed to send negative acknowledgment", "error", nakErr) + } return }Also applies to: 249-249, 256-256
🧹 Nitpick comments (4)
docker-comose.k8s.yml (1)
1-1: Fix the filename typo.The filename
docker-comose.k8s.ymlcontains a typo and should bedocker-compose.k8s.yml.helm/hades/values.yaml (1)
66-73: Consider enabling file store for production persistence.The file store configuration is commented out, leaving only memory store enabled. For production deployments, consider enabling file store to ensure message persistence across restarts.
ansible/hades/defaults/main.yml (1)
16-19: NATS configuration defaults are appropriate.The migration from Redis to NATS variables is complete. The defaults are suitable for development environments.
For production deployments, consider:
- Enabling TLS by default
- Using a more specific NATS URL instead of localhost
- Documenting the requirement for credentials in production
HadesScheduler/docker/docker.go (1)
171-171: Remove duplicate debug logging.Line 171 duplicates the error logging from line 170. Remove the duplicate.
if err != nil { job_logger.Error("Failed to execute job", slog.Any("error", err)) - job_logger.Debug("Failed to execute job", slog.Any("error", err)) return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
HadesAPI/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (23)
.env.example(1 hunks).vscode/tasks.json(1 hunks)HadesAPI/go.mod(3 hunks)HadesAPI/monitoring.go(1 hunks)HadesAPI/router.go(1 hunks)HadesAPI/router_test.go(3 hunks)HadesScheduler/docker/docker.go(4 hunks)HadesScheduler/docker/volume.go(1 hunks)HadesScheduler/main.go(3 hunks)ansible/hades/README.md(3 hunks)ansible/hades/defaults/main.yml(1 hunks)ansible/hades/templates/docker-compose-api.yml.j2(1 hunks)ansible/hades/templates/docker-compose-scheduler.yml.j2(1 hunks)ansible/hades/templates/hades.env.j2(1 hunks)compose.yml(2 hunks)docker-comose.k8s.yml(1 hunks)docs/api/Create Build Job (Test Fail).bru(3 hunks)docs/api/Create Build Job (Test Succeed - Result Container).bru(3 hunks)docs/api/Create Build Job (Test Succeed).bru(2 hunks)docs/api/Create Build Job (long-running-debug).bru(2 hunks)helm/hades/values.yaml(1 hunks)shared/utils/config.go(1 hunks)shared/utils/queue.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- .vscode/tasks.json
- HadesScheduler/docker/volume.go
- .env.example
- ansible/hades/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- compose.yml
- docs/api/Create Build Job (Test Succeed).bru
- HadesAPI/router.go
- HadesAPI/monitoring.go
- shared/utils/config.go
- HadesScheduler/main.go
- HadesAPI/go.mod
🧰 Additional context used
🧠 Learnings (2)
HadesAPI/router_test.go (1)
Learnt from: Mtze
PR: ls1intum/hades#220
File: HadesAPI/go.mod:10-10
Timestamp: 2025-06-16T18:38:47.219Z
Learning: When providing version information for Go modules, always verify the information carefully as incorrect version claims can mislead users. The NATS Go client (github.com/nats-io/nats.go) follows semantic versioning and remains in the v1.x series, not v2.x as might be incorrectly suggested by some search results.
shared/utils/queue.go (1)
Learnt from: Mtze
PR: ls1intum/hades#220
File: HadesAPI/go.mod:10-10
Timestamp: 2025-06-16T18:38:47.219Z
Learning: When providing version information for Go modules, always verify the information carefully as incorrect version claims can mislead users. The NATS Go client (github.com/nats-io/nats.go) follows semantic versioning and remains in the v1.x series, not v2.x as might be incorrectly suggested by some search results.
🧬 Code Graph Analysis (2)
HadesAPI/router_test.go (3)
shared/utils/queue.go (3)
HadesProducer(22-26)SetupNatsConnection(36-66)NewHadesProducer(69-105)HadesAPI/main.go (1)
HadesProducer(25-25)shared/utils/config.go (1)
NatsConfig(12-17)
shared/utils/queue.go (5)
shared/utils/prio.go (5)
Priority(5-5)HighPriority(8-8)MediumPriority(9-9)LowPriority(10-10)PrioritySubject(14-16)shared/utils/config.go (1)
NatsConfig(12-17)HadesAPI/main.go (1)
HadesProducer(25-25)HadesScheduler/main.go (1)
HadesConsumer(30-30)shared/payload/payload.go (1)
QueuePayload(15-21)
🪛 golangci-lint (1.64.8)
shared/utils/queue.go
242-242: Error return value of msg.Nak is not checked
(errcheck)
249-249: Error return value of msg.Nak is not checked
(errcheck)
256-256: Error return value of msg.Nak is not checked
(errcheck)
🪛 YAMLlint (1.37.1)
helm/hades/values.yaml
[error] 75-75: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: build (./HadesCloneContainer/Dockerfile, ls1intum/hades/hades-clone-container) / Build Docker Image for ls1intum/hades/hades-clone-container
- GitHub Check: build (./HadesCloneContainer/Dockerfile, ls1intum/hades/hades-clone-container) / Build Docker Image for ls1intum/hades/hades-clone-container
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
🔇 Additional comments (20)
docker-comose.k8s.yml (1)
7-7: NATS configuration looks correct.The NATS URL format is appropriate for container-to-container communication within the Docker Compose network.
ansible/hades/templates/hades.env.j2 (1)
1-4: NATS configuration variables are comprehensive and secure.The addition of NATS URL, username, password, and TLS configuration provides a complete and secure messaging setup. The template variable naming follows a consistent pattern.
ansible/hades/templates/docker-compose-scheduler.yml.j2 (1)
14-17: NATS environment variables are consistent across configuration files.The NATS configuration matches the pattern established in other template files and provides all necessary connection parameters for secure messaging.
docs/api/Create Build Job (long-running-debug).bru (2)
16-21: Shared volume configuration is appropriate for job execution.The addition of an emptyDir volume named "shared" provides a workspace for build steps to share data and artifacts.
32-35: Volume mount and working directory configuration is consistent.The volume mount at "/shared" and corresponding working directory change align with the shared workspace pattern.
docs/api/Create Build Job (Test Fail).bru (2)
16-21: Shared volume configuration is consistent with other API documentation files.The emptyDir volume configuration matches the pattern established in other build job examples.
32-35: Volume mount configuration aligns with shared workspace pattern.The volume mount and working directory changes are consistent with the shared workspace approach.
helm/hades/values.yaml (1)
1-4: Clarify namespace creation behavior.The namespace creation is disabled (
create: false) but a name is specified. This expects the namespace to already exist. Consider documenting this requirement or enabling namespace creation for smoother deployments.ansible/hades/templates/docker-compose-api.yml.j2 (1)
10-17: NATS migration looks good!The migration from Redis to NATS is properly implemented with:
- Correct JetStream enablement
- Proper health check configuration
- Appropriate environment variable updates
Also applies to: 22-35
docs/api/Create Build Job (Test Succeed - Result Container).bru (2)
54-54: Verify impact of strict error handling change.The script execution changed from error-tolerant (
set +ewith|| exit 0) to strict error handling (set -e). This means any command failure will now terminate the build immediately. Ensure this aligns with the expected behavior for test jobs.
16-21: Shared volume configuration looks good!The addition of shared volume and proper volume mounts enables data sharing between build steps, which is a common requirement for CI/CD pipelines.
Also applies to: 32-35
HadesAPI/router_test.go (1)
25-25: Test migration to NATS is well implemented!The test suite properly:
- Uses a specific NATS version (avoiding "latest")
- Sets up JetStream with appropriate health checks
- Properly manages connection lifecycle
- Correctly initializes the HadesProducer for tests
Also applies to: 39-80, 84-93
HadesScheduler/docker/docker.go (3)
36-36: LGTM!The addition of the
cleanupSharedVolumesboolean field to theDockerPropsstruct is well-placed and follows the existing pattern.
93-101: LGTM!The setter method is well-implemented with appropriate warning logging when cleanup is enabled. The method follows the builder pattern correctly.
175-175: LGTM!The debug logging for successful job execution is a good addition for troubleshooting.
shared/utils/queue.go (5)
51-54: LGTM!The TLS configuration now properly specifies a minimum version (TLS 1.2), which addresses the security concern from previous reviews.
69-105: LGTM!The
NewHadesProducerfunction is well-implemented with proper error handling and logging. The JetStream stream and KeyValue store configuration is appropriate for the job queue use case.
107-148: LGTM!The
NewHadesConsumerfunction properly creates consumers for each priority level and configures them with durable names for persistence. The consumer configuration is appropriate for the job processing pattern.
173-174: LGTM!The error handling for
hp.kv.Puthas been properly implemented, addressing the concern from previous reviews.
177-279: LGTM!The
DequeueJobfunction implements a sophisticated worker pool with proper concurrency control, priority-based message fetching, and context cancellation support. The implementation correctly handles the separation of message metadata (UUID) from payload data stored in the KeyValue store.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
HadesScheduler/go.mod (1)
46-47:nkeys/nuidlisted explicitly as indirect – rungo mod tidyto see if they can be dropped.Both packages are transitive dependencies of
nats.go; they usually don’t need to be listed in the require block unless you import them directly.
Runninggo mod tidyafter the refactor may automatically prune these lines, keeping thego.modminimal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
HadesScheduler/go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (1)
HadesScheduler/go.mod(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
HadesScheduler/go.mod (1)
Learnt from: Mtze
PR: ls1intum/hades#220
File: HadesAPI/go.mod:10-10
Timestamp: 2025-06-16T18:38:47.219Z
Learning: When providing version information for Go modules, always verify the information carefully as incorrect version claims can mislead users. The NATS Go client (github.com/nats-io/nats.go) follows semantic versioning and remains in the v1.x series, not v2.x as might be incorrectly suggested by some search results.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (./HadesCloneContainer/Dockerfile, ls1intum/hades/hades-clone-container) / Build Docker Image for ls1intum/hades/hades-clone-container
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: build (./HadesCloneContainer/Dockerfile, ls1intum/hades/hades-clone-container) / Build Docker Image for ls1intum/hades/hades-clone-container
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
- GitHub Check: Analyze (go)
|
@ShuaiweiYu I merged the nats develop branch - can you please have a look at the conflicts? |
# Conflicts: # HadesScheduler/go.mod # HadesScheduler/go.sum # HadesScheduler/main.go # compose.yml # go.work.sum # shared/go.mod # shared/go.sum # shared/utils/queue.go
Co-authored-by: Matthias Linhuber <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
helm/hades/Chart.yaml (1)
29-29: Add a trailing newline to silence YAMLlintThe file is missing a final newline, causing the reported
new-line-at-end-of-filewarning.- repository: https://nats-io.github.io/k8s/helm/charts/ + repository: https://nats-io.github.io/k8s/helm/charts/ +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
helm/hades/Chart.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
helm/hades/Chart.yaml
[error] 29-29: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build (./HadesCloneContainer/Dockerfile, ls1intum/hades/hades-clone-container) / Build Docker Image for ls1intum/hades/hades-clone-container
- GitHub Check: build (./HadesCloneContainer/Dockerfile, ls1intum/hades/hades-clone-container) / Build Docker Image for ls1intum/hades/hades-clone-container
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
helm/hades/Chart.yaml (2)
1-24: Chart metadata LGTMChart API version, name, description, type, chart version and appVersion are set correctly and follow Helm conventions.
26-29: Confirmed availability of NATS Helm chart version 1.3.7
The upstream index lists v1.3.9, v1.3.8, v1.3.7, sohelm dependency buildwill succeed as-is.• File: helm/hades/Chart.yaml (Lines 26–29)
dependencies: - name: nats version: 1.3.7 repository: https://nats-io.github.io/k8s/helm/charts/No changes needed.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
helm/hades/values.yaml (1)
4-5:latestimage tags were already flagged – please pin to a version tag
Same concern as in the previous review; leaving it unchanged makes roll-backs unpredictable.Also applies to: 22-23
helm/hades/templates/hades-scheduler-deployment.yaml (1)
34-37: Use built-in.Release.Namespaceinstead of custom valueAvoids duplication and guarantees correctness even if users install the chart into a different namespace.
- - name: K8S_NAMESPACE - value: {{ .Values.namespace.name }} + - name: K8S_NAMESPACE + value: "{{ .Release.Namespace }}"
🧹 Nitpick comments (6)
helm/hades/templates/hades-api-service.yaml (1)
1-12: Fix missing newline at end of file.The YAML file is missing a newline character at the end, which violates YAML formatting standards.
type: {{ .Values.hadesApi.service.type }} +helm/hades/templates/hades-api-deployment.yaml (1)
30-30: Fix missing newline at end of file.The YAML file is missing a newline character at the end, which violates YAML formatting standards.
value: "nats://{{ .Values.nats.host }}:{{ .Values.nats.port }}" +helm/hades/templates/hades-scheduler-service.yaml (1)
12-12: Fix missing newline at end of file.The YAML file is missing a newline character at the end, which violates YAML formatting standards.
type: {{ .Values.hadesScheduler.service.type }} +helm/hades/templates/ingress.yaml (1)
3-6: Specify namespace explicitly to avoid accidental cross-namespace installationIf the release is installed with
--namespaceHelm will inject it automatically, but being explicit improves manifest portability (e.g.kubectl apply -f …).metadata: name: "hades-ingress" + namespace: {{ .Release.Namespace }}helm/hades/values.yaml (1)
70-71: Add a terminating newlineSeveral linters (incl. YAMLlint) complain; append a blank line at EOF.
helm/hades/templates/hades-scheduler-deployment.yaml (1)
37-37: Missing newline at EOFAdd one to silence YAML lint errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
HadesScheduler/k8s/clusterAccess.go(1 hunks)HadesScheduler/k8s/k8s.go(1 hunks)helm/hades/templates/hades-api-deployment.yaml(1 hunks)helm/hades/templates/hades-api-service.yaml(1 hunks)helm/hades/templates/hades-scheduler-deployment.yaml(1 hunks)helm/hades/templates/hades-scheduler-role.yaml(1 hunks)helm/hades/templates/hades-scheduler-rolebinding.yaml(1 hunks)helm/hades/templates/hades-scheduler-service.yaml(1 hunks)helm/hades/templates/hades-scheduler-serviceaccount.yaml(1 hunks)helm/hades/templates/ingress.yaml(1 hunks)helm/hades/values.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- helm/hades/templates/hades-scheduler-serviceaccount.yaml
- helm/hades/templates/hades-scheduler-role.yaml
- helm/hades/templates/hades-scheduler-rolebinding.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
helm/hades/templates/hades-api-deployment.yaml
[error] 30-30: no new line character at the end of file
(new-line-at-end-of-file)
helm/hades/templates/hades-api-service.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
helm/hades/templates/hades-scheduler-deployment.yaml
[error] 37-37: no new line character at the end of file
(new-line-at-end-of-file)
helm/hades/templates/hades-scheduler-service.yaml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
helm/hades/templates/ingress.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
helm/hades/values.yaml
[error] 71-71: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
helm/hades/templates/hades-api-service.yaml (1)
6-7: LGTM: Service selector configuration is correct.The selector properly targets pods with the
hades-api-selectorlabel, which should match the corresponding deployment template.HadesScheduler/k8s/k8s.go (1)
83-86: LGTM: Proper in-cluster authentication implementation.The change correctly implements service account-based authentication for running the scheduler inside Kubernetes. This replaces the previous placeholder that logged a warning and returned an empty Scheduler.
helm/hades/templates/hades-api-deployment.yaml (2)
28-30: NATS integration configured correctly.The NATS_URL environment variable is properly configured using Helm values, enabling the API to connect to the messaging system.
19-25: Resource configuration follows best practices.Both resource limits and requests are properly configured with CPU and memory constraints, ensuring proper resource management in Kubernetes.
HadesScheduler/k8s/clusterAccess.go (1)
58-71: LGTM: Well-implemented in-cluster authentication.The
initializeInCluster()function correctly implements Kubernetes in-cluster configuration using service account credentials. The error handling is appropriate, logging errors and returning nil instead of panicking, which allows the calling code to handle failures gracefully.helm/hades/templates/hades-scheduler-service.yaml (1)
6-7: LGTM: Service selector correctly configured.The selector properly targets pods with the
hades-scheduler-selectorlabel, which should match the corresponding scheduler deployment template.
helm/hades/values.yaml
Outdated
| executor: "k8s" | ||
|
|
||
| rbac: |
There was a problem hiding this comment.
Property referenced in templates is missing – chart will render invalid YAML
helm/hades/templates/hades-scheduler-deployment.yaml uses .Values.namespace.name, but this key is not present here. Rendering will leave the env-var empty.
Add a namespace section or switch to {{ .Release.Namespace }} inside the template.
+# Namespace used by some templates
+namespace:
+ name: hades📝 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.
| executor: "k8s" | |
| rbac: | |
| executor: "k8s" | |
| # Namespace used by some templates | |
| namespace: | |
| name: hades | |
| rbac: |
🤖 Prompt for AI Agents
In helm/hades/values.yaml around lines 36 to 38, the values file is missing the
namespace.name property referenced in the hades-scheduler-deployment.yaml
template, causing invalid YAML rendering with an empty env-var. To fix this,
either add a namespace section with a name key in values.yaml or modify the
template to use {{ .Release.Namespace }} instead of .Values.namespace.name to
ensure the namespace is correctly set.
| - name: NATS_URL | ||
| value: "nats://{{ .Values.nats.host }}:{{ .Values.nats.port }}" | ||
| - name: HADES_EXECUTOR | ||
| value: {{ .Values.hadesScheduler.executor }} | ||
| - name: K8S_CONFIG_MODE |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote Helm substitutions to prevent accidental type-casting
If a future value (e.g. executor: 123) is numeric, unquoted output will be an int and break the env section. Safer to quote.
- - name: HADES_EXECUTOR
- value: {{ .Values.hadesScheduler.executor }}
+ - name: HADES_EXECUTOR
+ value: "{{ .Values.hadesScheduler.executor }}"📝 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.
| - name: NATS_URL | |
| value: "nats://{{ .Values.nats.host }}:{{ .Values.nats.port }}" | |
| - name: HADES_EXECUTOR | |
| value: {{ .Values.hadesScheduler.executor }} | |
| - name: K8S_CONFIG_MODE | |
| - name: NATS_URL | |
| value: "nats://{{ .Values.nats.host }}:{{ .Values.nats.port }}" | |
| - name: HADES_EXECUTOR | |
| value: "{{ .Values.hadesScheduler.executor }}" | |
| - name: K8S_CONFIG_MODE |
🤖 Prompt for AI Agents
In helm/hades/templates/hades-scheduler-deployment.yaml around lines 30 to 34,
the Helm template substitution for the environment variable HADES_EXECUTOR is
unquoted, which can cause type-casting issues if the value is numeric. To fix
this, wrap the substitution {{ .Values.hadesScheduler.executor }} in double
quotes to ensure the output is always treated as a string in the env section.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
helm/hades/templates/hades-scheduler-deployment.yaml (1)
32-33: QuoteHADES_EXECUTORvalue to avoid accidental type-casting
This was pointed out earlier but is still unaddressed: if the value invalues.yamlis numeric (e.g.executor: 123) the rendered YAML will contain an int, breaking the env section.- - name: HADES_EXECUTOR - value: {{ .Values.hadesScheduler.executor }} + - name: HADES_EXECUTOR + value: "{{ .Values.hadesScheduler.executor }}"
🧹 Nitpick comments (3)
helm/hades/templates/hades-scheduler-deployment.yaml (1)
37-37: Add newline at end of file
Missing trailing newline triggers YAML-lint (new-line-at-end-of-file) and can cause issues with some tooling.- value: {{ .Release.Namespace }} + value: {{ .Release.Namespace }} +helm/hades/readme.md (2)
59-63: Specify language for fenced code block
Markdown-lint flags this (MD040). Addingtextkeeps highlighting neutral while satisfying the rule.-``` +```text INFO Connected to NATS server url=nats://hades-nats.hades.svc:4222 INFO Started HadesScheduler in Kubernetes mode INFO Using service account for Kubernetes access--- `14-18`: **Fix list indentation to silence `MD007`** Indent the sub-list by two spaces to match markdownlint expectations. ```diff - * [Values Reference](#values-reference) + * [Values Reference](#values-reference)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
helm/hades/readme.md(1 hunks)helm/hades/templates/hades-scheduler-deployment.yaml(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
helm/hades/readme.md
14-14: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
15-15: Link fragments should be valid
(MD051, link-fragments)
59-59: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 YAMLlint (1.37.1)
helm/hades/templates/hades-scheduler-deployment.yaml
[error] 37-37: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
helm/hades/templates/hades-scheduler-deployment.yaml (2)
32-33: Quote Helm substitution to avoid accidental type-casting
Unquoted substitutions may be rendered as bare words or numbers (e.g.123) which the YAML parser will interpret as non-string types, breaking the container spec if the value is numeric.- - name: HADES_EXECUTOR - value: {{ .Values.hadesScheduler.executor }} + - name: HADES_EXECUTOR + value: "{{ .Values.hadesScheduler.executor }}"
34-35: Mirror the quoting pattern forconfigMode
For consistency and the same type-safety reasons as above, wrap the substitution in quotes.- - name: K8S_CONFIG_MODE - value: {{ .Values.hadesScheduler.configMode }} + - name: K8S_CONFIG_MODE + value: "{{ .Values.hadesScheduler.configMode }}"helm/hades/values.yaml (1)
4-4: Avoidlatestimage tags in production deployments
Usinglatestbreaks reproducibility and roll-backs. Pin to a concrete, immutable version published to GHCR.- tag: latest + tag: "vX.Y.Z" # <-- replace with the exact version you want to deployAlso applies to: 22-22
🧹 Nitpick comments (2)
helm/hades/templates/hades-scheduler-deployment.yaml (1)
37-37: Add trailing newline to satisfy YAML linters
A missing final newline trips up CI style checks such asyamllint.- value: {{ .Release.Namespace }} \ No newline at end of file + value: {{ .Release.Namespace }} +helm/hades/values.yaml (1)
71-72: Terminate file with a newline
Fixes thenew-line-at-end-of-filewarning fromyamllint.- port: 4222 \ No newline at end of file + port: 4222 +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
helm/hades/templates/hades-scheduler-deployment.yaml(1 hunks)helm/hades/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
helm/hades/templates/hades-scheduler-deployment.yaml
[error] 37-37: no new line character at the end of file
(new-line-at-end-of-file)
helm/hades/values.yaml
[error] 72-72: no new line character at the end of file
(new-line-at-end-of-file)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
- GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build Docker Image for ls1intum/hades/hades-api
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build Docker Image for ls1intum/hades/hades-scheduler
- GitHub Check: Analyze (go)
helm/hades/readme.md
Outdated
|
|
||
| ```bash | ||
| # 1. Create target namespace if the namespace does not exist | ||
| kubectl create namespace hades |
There was a problem hiding this comment.
The --create-namespace in the next step makes this obsolete
helm/hades/readme.md
Outdated
|
|
||
| # 2. Install the chart using serviceaccount mode (default) | ||
| helm upgrade --install hades ./helm/hades -n hades --create-namespace | ||
|
|
There was a problem hiding this comment.
When following this tutorial (with the previous things fixed) i get the following error:
2025/07/22 19:52:24 INFO Creating namespace in Kubernetes
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0xc8 pc=0x113aae4]
goroutine 1 [running]:
k8s.io/client-go/kubernetes.(*Clientset).CoreV1(...)
/go/pkg/mod/k8s.io/[email protected]/kubernetes/clientset.go:323
github.com/ls1intum/hades/hadesScheduler/k8s.createNamespace({0x168e580, 0x1f61520}, 0x0, {0xc000046026, 0xa})
/app/HadesScheduler/k8s/namespace.go:17 +0xe4
github.com/ls1intum/hades/hadesScheduler/k8s.NewK8sScheduler()
/app/HadesScheduler/k8s/k8s.go:55 +0x1cf
main.main()
/app/HadesScheduler/main.go:64 +0x3ca
There was a problem hiding this comment.
This is happening because the code of Hades-Scheduler has been changed, and therefore, a new image is built with tag pr-225. However, to ensure the helm could work correctly after merging, I changed the tag to the latest when I was done with testing. Can you run helm upgrade --install hades ./helm/hades -n hades --create-namespace \ --set hadesScheduler.image.tag=pr-225 ?
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
helm/hades/readme.md (5)
14-17: Fix TOC indentation and broken anchorThe nested “Values Reference” entry is over-indented (MD007) and the “Upgrade & Rollback” link points to a non-existent anchor (MD051).
- * [Values Reference](#values-reference) -* [Upgrade & Rollback](#upgrade--rollback) + * [Values Reference](#values-reference) +* [Upgrade](#upgrade)
27-27: Grammar tweak for hades-api description-| **hades-api** | Processes and validates the request and produce the build request as NATS events | +| **hades-api** | Processes and validates requests and produces build requests as NATS events |
65-67: Typo in--setexample
my-secrect→my-secret.- --set tls.secretName=my-secrect + --set tls.secretName=my-secret
80-80: Minor wording fix-> You maybe have to wait a few seconds until the NATS broker is set up. +> You may have to wait a few seconds until the NATS broker is set up.
84-88: Add language hint to fenced log block (MD040)-``` +```text INFO Connected to NATS server url=nats://hades-nats.hades.svc:4222 INFO Started HadesScheduler in Kubernetes mode INFO Using service account for Kubernetes access</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between d26bb037eaf85277e0c69e2204cbc5730f9778fa and ecf2134b939ddf418b24e4dd3dc59fcb9600adc5. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `helm/hades/readme.md` (1 hunks) </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🪛 LanguageTool</summary> <details> <summary>helm/hades/readme.md</summary> [grammar] ~80-~80: Ensure spelling is correct Context: ...deploy/hades-scheduler -f ``` > You maybe have to wait a few seconds until the NA... (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) </details> </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> <details> <summary>helm/hades/readme.md</summary> 14-14: Unordered list indentation Expected: 2; Actual: 4 (MD007, ul-indent) --- 15-15: Link fragments should be valid (MD051, link-fragments) --- 84-84: Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)</summary> * GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build linux/amd64 Docker Image for ls1intum/hades/hades-scheduler * GitHub Check: build (./HadesScheduler/Dockerfile, ls1intum/hades/hades-scheduler) / Build linux/arm64 Docker Image for ls1intum/hades/hades-scheduler * GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build linux/arm64 Docker Image for ls1intum/hades/hades-api * GitHub Check: build (./HadesAPI/Dockerfile, ls1intum/hades/hades-api) / Build linux/amd64 Docker Image for ls1intum/hades/hades-api * GitHub Check: Analyze (go) </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Summary by CodeRabbit
New Features
Chores
.gitignoreand.helmignorefiles to streamline chart packaging and version control.Bug Fixes