chore: establish project vision and craftsmanship principles#205
Open
chore: establish project vision and craftsmanship principles#205
Conversation
This commit includes a major refactoring to improve consistency, security, and maintainability across all Helm charts. ## Critical Fixes - Fix hard-coded replicas in commento (now configurable via replicaCount) - Fix hard-coded replicas in amundsen neo4j (now configurable via neo4j.replicas) - Fix hard-coded imagePullPolicy in amundsen frontend, metadata, and search services - Add missing imagePullPolicy to search deployment - Add config checksum annotations to trigger pod restarts on config changes: * amundsen neo4j deployment (for configmap) * commento deployment (for secret) * clickhouse-keeper statefulset (for configmap) * zeppelin deployment (for configmap) ## Template Standardization - Migrate all 'indent | trim' to modern 'nindent' style across all charts - Fix unquoted environment variable values in commento deployment - Standardize indentation and formatting for better readability - Consistent use of toYaml with nindent throughout ## API Migration - Migrate amundsen from Helm v1 to v2 API (1.0.0 -> 1.1.0) - Migrate commento from Helm v1 to v2 API (0.1.0 -> 0.2.0) - Migrate pgbouncer from Helm v1 to v2 API (1.0.8 -> 1.1.0) - Migrate spark-shuffle from Helm v1 to v2 API (0.1.0 -> 0.2.0) - Move dependencies from requirements.yaml to Chart.yaml - Update appVersion to reflect actual application versions - Fix typo: "folked" -> "forked" and "deamon" -> "daemon" ## Documentation - Add comprehensive CONTRIBUTING.md with chart standards and best practices - Add SECURITY.md with security policy and guidelines - Add .yamllint configuration for YAML linting - Make doc.sh executable ## Charts Modified - amundsen: replicas, imagePullPolicy, indentation, API v2, version bump - commento: replicas, indentation, quoting, checksums, API v2, version bump - clickhouse-keeper: config checksums - pgbouncer: API v2, version bump - spark-shuffle-service: API v2, version bump - zeppelin: config checksums This refactoring establishes patterns and standards that will make future maintenance easier and guide users toward production-ready deployments.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Summary of ChangesHello @duyet, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents a significant refactoring effort across multiple Helm charts, focusing on enhancing their consistency, security, and maintainability. The changes aim to standardize templating practices, ensure critical configurations are dynamic, and provide clear guidelines for future development and secure deployments. By migrating to Helm v2 API and introducing robust documentation, the project establishes a solid foundation for easier chart management and more reliable, production-ready deployments. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Collaborator
|
Testing on Kubernetes v1.29.10, Helm v3.14.0 ... (Run 19404758000) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.27.16, Helm v3.14.0 ... (Run 19404758000) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.30.6, Helm v3.14.0 ... (Run 19404758000) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.28.13, Helm v3.14.0 ... (Run 19404758000) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.31.2, Helm v3.14.0 ... (Run 19404758000) Linting chart failure! 🙅Testing chart failure! 🙅 |
Establishes the foundational document for this Helm charts repository, capturing the project's philosophy, development standards, and best practices. Key additions: - Core principles: simplicity, consistency, security, production-readiness - Semantic commit message standards and examples - Template standards and common patterns - values.yaml structure conventions - Chart versioning guidelines (semver) - Testing workflow and review process - Common patterns (ServiceAccount, SecurityContext, PDB) This document serves as the "soul" of the repository, guiding all future development and ensuring consistency across charts. The semantic commit convention ensures: - Clear communication through commit messages - Easy changelog generation - Better understanding of change impact - Consistent commit history
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring across multiple Helm charts, establishing a clear vision and craftsmanship principles for the project. The introduction of CONTRIBUTING.md, SECURITY.md, and .yamllint provides a strong foundation for future contributions. The migration to Helm v2 API, standardization on nindent, removal of hard-coded values, and addition of checksum annotations are all excellent improvements that enhance maintainability and security.
My review has identified a critical issue with the implementation of pod annotation merging in several templates, which will cause rendering failures. I've provided suggestions to fix this structural YAML issue. I also found a minor point of clarification in the new CONTRIBUTING.md documentation.
Overall, this is a fantastic contribution that will greatly improve the quality of the charts.
Comment on lines
21
to
25
| annotations: | ||
| {{ toYaml . | indent 8 }} | ||
| checksum/config: {{ include (print $.Template.BasePath "/neo4j-configmap.yaml") . | sha256sum }} | ||
| {{- with default .Values.podAnnotations .Values.neo4j.podAnnotations }} | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
The placement of the with block for podAnnotations is incorrect. It's currently a sibling of the annotations block within metadata, which will result in an invalid YAML structure. The podAnnotations should be merged into the annotations block by indenting the with block to be a child of annotations.
annotations:
checksum/config: {{ include (print $.Template.BasePath "/neo4j-configmap.yaml") . | sha256sum }}
{{- with default .Values.podAnnotations .Values.neo4j.podAnnotations }}
{{- toYaml . | nindent 8 }}
{{- end }}
Comment on lines
18
to
22
| annotations: | ||
| checksum/config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }} | ||
| {{- with .Values.podAnnotations }} | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
The with block for podAnnotations is incorrectly placed. It should be inside the annotations block to ensure the generated YAML is valid. Currently, it's a sibling of the annotations block, which will lead to a malformed Kubernetes manifest.
annotations:
checksum/config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }}
{{- with .Values.podAnnotations }}
{{- toYaml . | nindent 8 }}
{{- end }}
Comment on lines
14
to
18
| annotations: | ||
| {{ toYaml .Values.podAnnotations | indent 8 | trim }} | ||
| checksum/secret: {{ include (print $.Template.BasePath "/secret.yaml") . | sha256sum }} | ||
| {{- if .Values.podAnnotations }} | ||
| {{ toYaml .Values.podAnnotations | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
The if block for podAnnotations is incorrectly placed. It should be inside the annotations block to merge the annotations correctly and produce valid YAML. As it is, it will generate an invalid structure.
annotations:
checksum/secret: {{ include (print $.Template.BasePath "/secret.yaml") . | sha256sum }}
{{- if .Values.podAnnotations }}
{{ toYaml .Values.podAnnotations | nindent 8 }}
{{- end }}
Comment on lines
19
to
23
| annotations: | ||
| checksum/config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }} | ||
| {{- with .Values.podAnnotations }} | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
The with block for podAnnotations is incorrectly indented, placing it outside the metadata block. This will cause a template rendering error. It should be moved inside the annotations block to correctly merge the annotations.
annotations:
checksum/config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }}
{{- with .Values.podAnnotations }}
{{- toYaml . | nindent 8 }}
{{- end }}| #### Chart Testing | ||
|
|
||
| - CI automatically tests charts using `chart-testing` | ||
| - Tests across multiple Kubernetes versions (1.27-1.31) |
There was a problem hiding this comment.
Kubernetes version 1.31 has not been released yet. It might be better to specify the current range of supported versions, for example, up to 1.30, to avoid confusion for contributors.
…links Improves the main README to provide better orientation for new users and contributors. Changes: - Add tagline: "Production-ready Helm Charts for Kubernetes" - Link to CLAUDE.md for project philosophy (read this first!) - Add complete list of available charts with descriptions - Add documentation section linking to CLAUDE.md, CONTRIBUTING.md, SECURITY.md - Improve "Using Helm" section with actual bash examples - Add "Contributing" section with clear steps - Reference semantic commit conventions This makes the repository more discoverable and user-friendly while guiding contributors to the right documentation.
Collaborator
|
Testing on Kubernetes v1.29.10, Helm v3.14.0 ... (Run 19404778162) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.31.2, Helm v3.14.0 ... (Run 19404778162) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.27.16, Helm v3.14.0 ... (Run 19404778162) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.28.13, Helm v3.14.0 ... (Run 19404778162) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.30.6, Helm v3.14.0 ... (Run 19404778162) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.30.6, Helm v3.14.0 ... (Run 19404786853) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.27.16, Helm v3.14.0 ... (Run 19404786853) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.29.10, Helm v3.14.0 ... (Run 19404786853) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.31.2, Helm v3.14.0 ... (Run 19404786853) Linting chart failure! 🙅Testing chart failure! 🙅 |
Collaborator
|
Testing on Kubernetes v1.28.13, Helm v3.14.0 ... (Run 19404786853) Linting chart failure! 🙅Testing chart failure! 🙅 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit includes a major refactoring to improve consistency, security, and maintainability across all Helm charts.
Critical Fixes
Template Standardization
API Migration
Documentation
Charts Modified
This refactoring establishes patterns and standards that will make future maintenance easier and guide users toward production-ready deployments.