-
Notifications
You must be signed in to change notification settings - Fork 10
feat(t8s-cluster/artifacthub): use centralised helmRepositories template #1846
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
feat(t8s-cluster/artifacthub): use centralised helmRepositories template #1846
Conversation
Summary of ChangesHello @cwrau, 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 introduces a significant refactoring to centralize the management and definition of Helm repositories within the 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
|
WalkthroughRefactors Helm repository handling by consolidating template and schema definitions. Replaces inline HelmRepository generation loop with a shared template include and moves schema validation to an external reference, centralizing repository configuration management. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 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 |
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.
Code Review
This pull request refactors the helmRepositories configuration to use a centralized template and schema, which is a great improvement for maintainability. However, the changes appear to be incomplete. The referenced common Helm template common.helm.repositories and the JSON schema definition helmRepositories are not present in the provided files. These omissions will lead to deployment and validation failures.
charts/t8s-cluster/templates/management-cluster/repositories/helmRepositories.yaml
Show resolved
Hide resolved
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/t8s-cluster/templates/management-cluster/repositories/helmRepositories.yaml(1 hunks)charts/t8s-cluster/values.schema.json(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/t8s-cluster/templates/management-cluster/repositories/helmRepositories.yaml
[error] 3-3: syntax error: expected '', but found '{'
(syntax)
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (1)
charts/t8s-cluster/templates/management-cluster/repositories/helmRepositories.yaml (1)
2-4: Verify that the included template expects the wrapped context signature.Line 3 passes context as
(dict "context" .), which means thecommon.helm.repositoriestemplate must access the dot context via.contextrather than directly via.. Ensure the template is designed to receive this wrapped dictionary signature.Additionally, YAMLlint reports a syntax error on line 3, which is likely a false positive from the linter not understanding Helm templating syntax. Verify this renders correctly in your cluster.
Please confirm:
- Does
common.helm.repositoriestemplate access the context via.contextor.?- Does the template render correctly when deployed to your cluster?
Alternatively, if the template expects the bare dot context, consider passing it directly:
- {{- include "common.helm.repositories" (dict "context" .) -}} + {{- include "common.helm.repositories" . -}}
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.
Pull request overview
This PR attempts to refactor the t8s-cluster chart to use centralized helmRepositories template and schema definitions from the common chart. However, the referenced template (common.helm.repositories) and schema definition ($defs/helmRepositories) do not exist in the common chart (version 1.6.0), making this PR incomplete and non-functional.
Key changes:
- Replaces inline schema definition with external reference to common chart schema
- Replaces inline template logic with call to centralized common template
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| charts/t8s-cluster/values.schema.json | Replaces inline helmRepositories schema with external reference to non-existent common schema definition |
| charts/t8s-cluster/templates/management-cluster/repositories/helmRepositories.yaml | Replaces inline template logic with call to non-existent common.helm.repositories template |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.