-
Notifications
You must be signed in to change notification settings - Fork 32
[DSD-9381]Updated helm charts, pustrigger,and dockerfile from develop… #193
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
Conversation
… branch Signed-off-by: bhumi46 <thisisbn46@gmail.com>
WalkthroughThis pull request adds platform specifications to the Keycloak build workflow, introduces a new Istio addon Helm chart for gateway and virtual service configuration, updates the keycloak-init chart with host configuration and label metadata, and simplifies the keycloak-artemis Dockerfile by removing theme deployments and build commands. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… branch Signed-off-by: bhumi46 <thisisbn46@gmail.com>
… branch Signed-off-by: bhumi46 <thisisbn46@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
helm/keycloak-init/values.yaml (1)
413-418: Add documentation for assign_client_scopes.The new
assign_client_scopesfield references client scopes defined earlier in the values file. Consider adding an inline comment to clarify this is linking to the scopes defined in theclient_scopessection.assign_client_scopes: + # These must match names defined in client_scopes section - update_oidc_client - add_oidc_client - get_certificate - upload_certificatehelm/keycloak-init/templates/configmap.yaml (1)
7-11: Add validation or documentation for host format requirements.The ConfigMap templates host values directly into URLs without format validation. If keycloakInternalHost or keycloakExternalHost are malformed (e.g., containing trailing slashes or missing domains), the resulting URLs will be invalid.
Consider adding inline comments documenting the expected format:
data: keycloak-internal-host: {{ .Values.keycloakInternalHost }} + # Expected format: service.namespace or service.namespace.svc.cluster.local keycloak-internal-url: {{ printf "http://%s" .Values.keycloakInternalHost }} keycloak-external-host: {{ .Values.keycloakExternalHost }} + # Expected format: fully qualified domain name (FQDN) keycloak-external-url: {{ printf "https://%s" .Values.keycloakExternalHost }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/push-trigger.yml(2 hunks)deploy/chart/istio-addons/Chart.yaml(1 hunks)deploy/chart/istio-addons/templates/gateway.yaml(1 hunks)deploy/chart/istio-addons/templates/virtualservice.yaml(1 hunks)deploy/chart/istio-addons/values.yaml(1 hunks)helm/keycloak-init/templates/configmap.yaml(1 hunks)helm/keycloak-init/values.yaml(2 hunks)keycloak-artemis/Dockerfile(0 hunks)
💤 Files with no reviewable changes (1)
- keycloak-artemis/Dockerfile
🧰 Additional context used
🪛 YAMLlint (1.37.1)
deploy/chart/istio-addons/templates/virtualservice.yaml
[error] 13-13: syntax error: could not find expected ':'
(syntax)
🔇 Additional comments (7)
deploy/chart/istio-addons/Chart.yaml (1)
1-24: Verify appVersion alignment.The chart descriptor looks standard and correct. However, confirm that
appVersion: "1.16.0"aligns with the version of the application or Istio being deployed..github/workflows/push-trigger.yml (1)
28-45: Verify that the referenced docker-build.yml workflow accepts the PLATFORMS input parameter.The push-trigger workflow now passes a PLATFORMS input to the docker-build.yml workflow located in the external mosip/kattu repository. Ensure that docker-build.yml is configured to accept this parameter and uses it correctly. Review the mosip/kattu repository's docker-build.yml workflow definition to confirm that PLATFORMS is defined as a valid input parameter.
helm/keycloak-init/templates/configmap.yaml (2)
18-27: All template helpers are properly defined or inherited.keycloak-init.fullnameis defined locally in_helpers.tpl, whilecommon.labels.standardandcommon.tplvalues.rendercome from the declared Bitnamicommonchart dependency (version 1.x.x in Chart.yaml). No action needed.
1-11: The review comment is based on incorrect file content. The actualconfigmap.yamldoes not create akeycloak-hostConfigMap as shown in the snippet. Instead, it creates akeycloak-init-configurationConfigMap containinginput.yamlwith realm configuration, which is properly mounted and used by the Job injob.yaml(line 76).Likely an incorrect or invalid review comment.
deploy/chart/istio-addons/templates/virtualservice.yaml (2)
11-14: File does not exist in repository.The file
deploy/chart/istio-addons/templates/virtualservice.yamlreferenced in this review comment does not exist in the repository. Nochart/oristio-addons/directory structure was found. This review comment cannot be applied to this codebase.Likely an incorrect or invalid review comment.
21-24: This review comment references files that do not exist in this repository.The file
deploy/chart/istio-addons/templates/virtualservice.yamland relatedgateway.yamldo not exist in the mosip/keycloak codebase. This review appears to have been intended for a different repository or has been applied to the wrong codebase. No verification of the TLS architecture claims is possible against non-existent files.Likely an incorrect or invalid review comment.
deploy/chart/istio-addons/templates/gateway.yaml (1)
8-12: Configure TLS/HTTPS for the Gateway.The Gateway currently exposes only HTTP on port 80. For production deployments, HTTPS should be explicitly configured unless TLS termination is handled by an upstream proxy or load balancer.
Verify that TLS is terminated upstream and confirm this is the intended architecture. If TLS should be configured at the Gateway level, add a TLS section with certificate references:
servers: - port: number: 80 name: http protocol: HTTP hosts: - {{ .Values.keycloakExternalHost }} + - port: + number: 443 + name: https + protocol: HTTPS + tls: + mode: SIMPLE + credentialName: keycloak-tls-cert + hosts: + - {{ .Values.keycloakExternalHost }}
… branch
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.