add dashboards as app #554
Conversation
dosubot
bot
added
the
size:L
WalkthroughThis pull request introduces a new Grafana dashboards Helm chart with comprehensive configuration options. The changes include creating a new Helm package for Grafana dashboards, adding configuration files like Changes
Sequence DiagramsequenceDiagram
participant User
participant HelmChart
participant GrafanaDashboard
participant Grafana
User->>HelmChart: Configure dashboards
HelmChart->>GrafanaDashboard: Create dashboard resources
GrafanaDashboard->>Grafana: Apply URL or JSON dashboards
Grafana-->>GrafanaDashboard: Confirm dashboard import
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (5)
packages/apps/grafana-dashboards/values.schema.json (1)
17-21: Add validation for instanceSelectorThe instanceSelector should validate the Kubernetes label selector format.
"instanceSelector": { "type": "object", "description": "Selector to match Grafana instances.", - "default": {} + "default": {}, + "properties": { + "matchLabels": { + "type": "object", + "description": "Label selector to match Grafana instances", + "patternProperties": { + "^[a-zA-Z0-9]([a-zA-Z0-9-_.])*$": { + "type": "string", + "pattern": "^[a-zA-Z0-9]([a-zA-Z0-9-_.])*$" + } + } + } + } }packages/apps/grafana-dashboards/templates/grafana-dashboards.yaml (1)
8-11: Add namespace to metadataAdd namespace to metadata to ensure proper resource placement.
kind: GrafanaDashboard metadata: name: {{ .name }} + namespace: {{ .Release.Namespace }}packages/apps/grafana-dashboards/values.yaml (2)
20-29: Enhance dashboard example with required fieldsThe current JSON dashboard example is oversimplified and missing required fields for a valid Grafana dashboard.
## Example: ## jsonDashboards: ## - name: "grafanadashboard-sample" ## json: | ## { +## "id": null, +## "title": "Sample Dashboard", +## "tags": ["generated", "sample"], +## "timezone": "browser", +## "schemaVersion": 16, ## "panels": [ -## { "type": "graph", "title": "Sample Panel" } +## { +## "id": 1, +## "type": "graph", +## "title": "Sample Panel", +## "datasource": "${DS_PROMETHEUS}" +## } ## ] ## }
31-34: Document instanceSelector behaviorAdd more context about the instanceSelector's role in dashboard distribution.
## @param instanceSelector Selector to match Grafana instances. -## Leave empty to skip instance selection. +## Specify labels to match Grafana instances where these dashboards should be deployed. +## If empty, dashboards will be matched to Grafana instances in the same namespace. +## For multi-tenant setups, use this to target specific Grafana instances. instanceSelector: {} # @param instanceSelector Selector for matching Grafana instancespackages/extra/monitoring/templates/grafana/grafana.yaml (1)
Line range hint
91-94: Improve template indentation for better readabilityThe conditional annotation block could be better indented for improved readability.
Consider this format:
metadata: annotations: - {{- if ne $issuerType "cloudflare" }} - acme.cert-manager.io/http01-ingress-class: "{{ $ingress }}" - {{- end }} + {{- if ne $issuerType "cloudflare" }} + acme.cert-manager.io/http01-ingress-class: "{{ $ingress }}" + {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/apps/grafana-dashboards/logos/grafana-dashboards.svgis excluded by!**/*.svg
📒 Files selected for processing (14)
packages/apps/grafana-dashboards/.helmignore(1 hunks)packages/apps/grafana-dashboards/Chart.yaml(1 hunks)packages/apps/grafana-dashboards/Makefile(1 hunks)packages/apps/grafana-dashboards/README.md(1 hunks)packages/apps/grafana-dashboards/templates/grafana-dashboards.yaml(1 hunks)packages/apps/grafana-dashboards/values.schema.json(1 hunks)packages/apps/grafana-dashboards/values.yaml(1 hunks)packages/apps/tenant/Chart.yaml(1 hunks)packages/apps/tenant/templates/tenant.yaml(1 hunks)packages/apps/versions_map(2 hunks)packages/core/installer/values.yaml(1 hunks)packages/extra/monitoring/Chart.yaml(1 hunks)packages/extra/monitoring/templates/grafana/grafana.yaml(1 hunks)packages/extra/versions_map(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- packages/extra/monitoring/Chart.yaml
- packages/apps/tenant/Chart.yaml
- packages/apps/grafana-dashboards/.helmignore
- packages/apps/grafana-dashboards/Makefile
- packages/apps/grafana-dashboards/Chart.yaml
- packages/apps/grafana-dashboards/README.md
- packages/core/installer/values.yaml
🧰 Additional context used
🪛 yamllint (1.35.1)
packages/apps/grafana-dashboards/templates/grafana-dashboards.yaml
[warning] 1-1: too many blank lines
(1 > 0) (empty-lines)
[error] 2-2: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (7)
packages/apps/versions_map (2)
16-16: Introduce and verify the newgrafana-dashboardsentry.The addition of
grafana-dashboards 0.1.0 HEADaligns with the introduction of the new Helm chart for Grafana dashboards. Consider confirming the newly added version is referenced properly in your chart configuration and codebase.
99-100: Ensure consistency with tenant chart updates.Updating the tenant from
1.6.5 f1e11451to1.6.6 HEADappears consistent with the rest of the pull request. Verify that all references or dependencies are updated accordingly (e.g.,Chart.yaml, any CI/CD pipelines, or documentation).packages/apps/tenant/templates/tenant.yaml (1)
244-244: Be cautious with wildcard resources.Allowing
*resources underapiGroups: ["apps.cozystack.io"]in the admin role significantly broadens access. This is normal for an admin role, but ensure you are comfortable with granting complete permissions in production environments.packages/extra/versions_map (1)
20-21: Confirm proper referencing of commit hash for “monitoring 1.5.3” and the stability of “monitoring 1.5.4 HEAD”.Switching from
HEADto a specific commitc1ca19dccan help track changes precisely, but ensure thatc1ca19dcreferences the valid updated build you intend to distribute. Additionally, usingHEADin production for version1.5.4could introduce unexpected changes unless carefully managed.packages/extra/monitoring/templates/grafana/grafana.yaml (3)
13-13: LGTM: Namespace-aware dashboard label enables multi-tenancyThe dynamic dashboard label using namespace value aligns well with multi-tenant dashboard management.
Let's verify the dashboard label usage:
✅ Verification successful
Verified: Dashboard label change is consistently implemented and properly integrated
The verification shows that:
- The namespace-based dashboard label is consistently used in both the main grafana configuration (
packages/extra/monitoring/templates/grafana/grafana.yaml) and the new grafana-dashboards app (packages/apps/grafana-dashboards/templates/grafana-dashboards.yaml)- The
instanceSelectorconfiguration is properly set up in the grafana-dashboards app, allowing for proper dashboard-to-grafana instance mapping- The change aligns with the grafana-operator CRD specifications that support instance selection for dashboards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the dashboard label is used consistently across the codebase # and particularly in the new grafana-dashboards app # Search for dashboard label references rg --type yaml 'dashboards:.*\$.*NS' # Look for dashboard selector configurations rg --type yaml 'instanceSelector|dashboardSelector'Length of output: 3999
Line range hint
91-116: Verify ingress security configurationThe conditional ingress annotation is correct, but let's ensure the overall ingress security configuration is robust.
Let's verify the ingress configuration:
✅ Verification successful
Ingress security configuration follows consistent patterns across the cluster
Based on the verification results, the ingress configuration in the Grafana template aligns with the cluster-wide security patterns:
- Consistent use of
cert-manager.io/cluster-issuer: letsencrypt-prodacross multiple services- Proper TLS configuration with dedicated secret names
- Standardized
ingressClassNameusage matching the cluster's ingress controller- Appropriate conditional handling for Cloudflare vs. non-Cloudflare setups
The implementation follows the same secure patterns used in other critical services like Keycloak, Seaweedfs, and the dashboard, indicating a robust and standardized approach to ingress security.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for consistent ingress security patterns across the cluster # Look for other ingress configurations using cert-manager rg --type yaml 'cert-manager\.io/(cluster-)?issuer' # Check for other instances using conditional cloudflare configuration rg --type yaml 'cloudflare.*ingress|ingress.*cloudflare' # Verify TLS configuration patterns rg --type yaml 'ingressClassName|tls:.*secretName'Length of output: 13254
Line range hint
1-6: Add error handling for resource lookupsThe template uses
lookupfunction to fetch critical configuration but doesn't handle cases where the ConfigMap or Namespace resources might not exist or lack required annotations.Consider adding validation using
requiredfunction:-{{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} +{{- $cozyConfig := required "ConfigMap 'cozystack' in namespace 'cozy-system' not found" (lookup "v1" "ConfigMap" "cozy-system" "cozystack") }} -{{- $myNS := lookup "v1" "Namespace" "" .Release.Namespace }} +{{- $myNS := required "Current namespace details not found" (lookup "v1" "Namespace" "" .Release.Namespace) }}
| "jsonDashboards": { | ||
| "type": "array", | ||
| "description": "List of dashboards with inline JSON.", | ||
| "default": [], | ||
| "items": {} | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance schema validation for jsonDashboards
Similar to urlDashboards, the items object needs proper validation for required fields and their types.
"jsonDashboards": {
"type": "array",
"description": "List of dashboards with inline JSON.",
"default": [],
- "items": {}
+ "items": {
+ "type": "object",
+ "required": ["name", "json"],
+ "properties": {
+ "name": {
+ "type": "string",
+ "description": "The name of the dashboard"
+ },
+ "json": {
+ "type": "object",
+ "description": "The JSON content of the dashboard"
+ }
+ },
+ "additionalProperties": false
+ }
}📝 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.
| "jsonDashboards": { | |
| "type": "array", | |
| "description": "List of dashboards with inline JSON.", | |
| "default": [], | |
| "items": {} | |
| }, | |
| "jsonDashboards": { | |
| "type": "array", | |
| "description": "List of dashboards with inline JSON.", | |
| "default": [], | |
| "items": { | |
| "type": "object", | |
| "required": ["name", "json"], | |
| "properties": { | |
| "name": { | |
| "type": "string", | |
| "description": "The name of the dashboard" | |
| }, | |
| "json": { | |
| "type": "object", | |
| "description": "The JSON content of the dashboard" | |
| } | |
| }, | |
| "additionalProperties": false | |
| } | |
| }, |
| "urlDashboards": { | ||
| "type": "array", | ||
| "description": "List of dashboards with URLs.", | ||
| "default": [], | ||
| "items": {} | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance schema validation for urlDashboards
The items object is empty, which means any object structure will be accepted. Add proper validation for required fields and their types.
"urlDashboards": {
"type": "array",
"description": "List of dashboards with URLs.",
"default": [],
- "items": {}
+ "items": {
+ "type": "object",
+ "required": ["name", "link"],
+ "properties": {
+ "name": {
+ "type": "string",
+ "description": "The name of the dashboard"
+ },
+ "link": {
+ "type": "string",
+ "format": "uri",
+ "description": "The URL of the dashboard JSON"
+ }
+ },
+ "additionalProperties": false
+ }
}📝 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.
| "urlDashboards": { | |
| "type": "array", | |
| "description": "List of dashboards with URLs.", | |
| "default": [], | |
| "items": {} | |
| }, | |
| "urlDashboards": { | |
| "type": "array", | |
| "description": "List of dashboards with URLs.", | |
| "default": [], | |
| "items": { | |
| "type": "object", | |
| "required": ["name", "link"], | |
| "properties": { | |
| "name": { | |
| "type": "string", | |
| "description": "The name of the dashboard" | |
| }, | |
| "link": { | |
| "type": "string", | |
| "format": "uri", | |
| "description": "The URL of the dashboard JSON" | |
| } | |
| }, | |
| "additionalProperties": false | |
| } | |
| } |
| resyncPeriod: "30s" | ||
| json: | | ||
| {{ .json | indent 4 }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate required fields before rendering
Add validation for required fields before rendering the template.
+ {{- if not .json }}
+ {{- fail "Dashboard json is required" }}
+ {{- end }}
resyncPeriod: "30s"
json: |
{{ .json | indent 4 }}📝 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.
| resyncPeriod: "30s" | |
| json: | | |
| {{ .json | indent 4 }} | |
| {{- if not .json }} | |
| {{- fail "Dashboard json is required" }} | |
| {{- end }} | |
| resyncPeriod: "30s" | |
| json: | | |
| {{ .json | indent 4 }} |
| {{- if .Values.instanceSelector | not }} | ||
| instanceSelector: | ||
| matchLabels: | ||
| dashboards: {{ $myNS }} | ||
| {{- else }} | ||
| instanceSelector: | ||
| {{ $instanceSelector | indent 4 }} | ||
| {{- end }} |
There was a problem hiding this comment.
Fix template variable references and indentation
There are several issues in the instanceSelector conditional block:
.Valuesis incorrectly referenced (should beValues)- Indentation is incorrect for the instanceSelector template
- {{- if .Values.instanceSelector | not }}
+ {{- if not .Values.instanceSelector }}
instanceSelector:
matchLabels:
dashboards: {{ $myNS }}
{{- else }}
instanceSelector:
-{{ $instanceSelector | indent 4 }}
+ {{- toYaml .Values.instanceSelector | nindent 4 }}
{{- end }}📝 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.
| {{- if .Values.instanceSelector | not }} | |
| instanceSelector: | |
| matchLabels: | |
| dashboards: {{ $myNS }} | |
| {{- else }} | |
| instanceSelector: | |
| {{ $instanceSelector | indent 4 }} | |
| {{- end }} | |
| {{- if not .Values.instanceSelector }} | |
| instanceSelector: | |
| matchLabels: | |
| dashboards: {{ $myNS }} | |
| {{- else }} | |
| instanceSelector: | |
| {{- toYaml .Values.instanceSelector | nindent 4 }} | |
| {{- end }} |
Summary by CodeRabbit
New Features
Version Updates
Improvements