-
-
Notifications
You must be signed in to change notification settings - Fork 506
Add user_kustomizations-map for specifying Kustomization-"sets" to be applied sequentially #1924
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
base: master
Are you sure you want to change the base?
Add user_kustomizations-map for specifying Kustomization-"sets" to be applied sequentially #1924
Conversation
Summary of ChangesHello @vsalomaki, 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 enhancement to the cluster bootstrapping process by implementing a "pre-install Kustomization" capability. This allows for the execution of specific Kubernetes manifests, such as Helm Charts or Custom Resource Definitions (CRDs), before the primary Kustomization application. This feature is crucial for scenarios where certain components, like CRDs, must be fully deployed and stable before dependent applications can be installed, providing greater control and flexibility in complex Kubernetes 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
|
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 introduces a pre-install step for Kustomize, allowing Helm charts and other manifests to be applied before the main extra-manifests. This is a useful feature for deploying resources with dependencies, like CRDs.
The implementation has a critical bug in kustomization_user.tf where the pre-install step is triggered by the wrong condition. Additionally, there's a design issue where both pre-install and post-install steps use the same remote directory, which can lead to conflicts and redundant operations. I've provided suggestions to fix these issues.
I've also included some minor suggestions to improve the clarity of the documentation in README.md.
2d6a255 to
c0ce730
Compare
|
/gemini review |
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 introduces a pre-install step for Kustomize, which is a great feature for handling dependencies like CRDs. The implementation is functional and the documentation updates are clear. However, the new Terraform code in kustomization_user.tf introduces significant code duplication. My main feedback is to refactor this duplicated logic into a reusable local module to improve long-term maintainability. I've also included a few other suggestions to improve robustness and documentation.
c0ce730 to
c9073c0
Compare
|
/gemini review |
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 introduces a significant and valuable feature: the ability to define multiple, ordered kustomization sets for pre- and post-install actions. This is achieved by refactoring the existing kustomization logic into new, more flexible Terraform modules (user_kustomizations and user_kustomization_set). The changes are well-structured and the documentation in README.md and kube.tf.example is clear.
My review focuses on improving the robustness of the new implementation. I've identified a critical issue where the deployment script can fail if an optional kustomization directory (like extra-manifests-preinstall) is not present, and an incorrect cleanup command that would also cause failures. The suggested changes will make the new feature more resilient and prevent errors in common use cases.
c9073c0 to
b91919c
Compare
|
/gemini review |
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 is a great refactoring that introduces a more flexible and powerful way to manage user kustomizations through ordered sets. The use of modules cleans up the root configuration and provides better encapsulation. The changes are well-documented and include helpful examples. I have a few suggestions to improve it further, mainly regarding a potential breaking change in variable types and a possible efficiency improvement.
2fcad6e to
e6b0924
Compare
|
/gemini review |
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 introduces a significant and valuable refactoring to support sequential Kustomization deployments, enabling pre-install actions before the main manifests are applied. This is achieved by introducing new Terraform modules (user_kustomizations and user_kustomization_set) that provide a more flexible and powerful way to manage custom deployments. The changes also improve security by marking sensitive variables appropriately.
My review focuses on the correctness and maintainability of this new implementation. I've found a high-severity issue where the default behavior for pre-install steps doesn't pass extra_kustomize_parameters as documented, which could break backward compatibility expectations. I've also identified a couple of medium-severity issues related to a typo in an example and a minor code redundancy that could be improved for better maintainability. Overall, this is a great enhancement, and with these fixes, it will be even more robust.
a76819b to
8f403bd
Compare
|
/gemini summary |
Summary of ChangesThis pull request significantly enhances the Kubernetes cluster bootstrapping process by introducing a flexible, sequential Kustomization deployment mechanism. It refactors existing pre- and post-install Kustomization logic into a modular, configurable system, allowing users to define multiple Kustomization steps with specific ordering and post-deployment actions. This change improves maintainability, addresses potential conflicts, and provides greater control over complex application deployments, particularly for scenarios requiring CRD installation before dependent applications. Highlights
Changelog
Activity
|
|
/gemini review |
mysticaltech
left a 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.
@vsalomaki Appreciate this. If everything checks out, I will keep it for the next major version as it's a breaking changes for people already using kustomization, unless i'm mistaken.
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 introduces a significant and well-executed refactoring to support sequential Kustomization deployments, including pre-install steps. The introduction of user_kustomizations and the new Terraform modules (user_kustomization_set, user_kustomizations) provides a much more flexible and powerful way to manage complex deployment ordering. The code is well-structured, and security considerations like marking sensitive variables have been addressed. I have a couple of suggestions to further improve the flexibility of the new mechanism by allowing steps that only contain commands, which is useful for inserting wait conditions between manifest applications.
Currently it's using the existing variables of Maybe the best course of action would be to "deprecate" the old variables and remove them later on? |
|
/gemini review |
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 introduces a significant and well-executed refactoring to support pre-install kustomizations. By replacing the previous null_resource-based implementation with new, modular Terraform modules (user_kustomization_set and user_kustomizations), the changes greatly improve the flexibility, readability, and maintainability of deploying custom manifests. The new user_kustomizations variable provides users with fine-grained control over the deployment sequence, including pre- and post-deployment actions for each step. The documentation and examples have been updated clearly to reflect these powerful new capabilities. My review includes a couple of suggestions to further improve the robustness of the new implementation.
|
/gemini review |
19e7281 to
58b45fe
Compare
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 introduces a significant and well-designed enhancement for deploying user-defined kustomizations. The refactoring into user_kustomization_set and user_kustomizations modules provides a flexible and robust mechanism for sequential deployments, which is a great improvement over the previous implementation. The code is well-structured, and the use of triggers and cleanup traps demonstrates attention to detail.
My review includes a high-severity suggestion to make the deployment script more robust against empty kustomization.yaml files, and a medium-severity suggestion to improve the clarity of an example in the documentation. Overall, this is an excellent contribution.
58b45fe to
453fa10
Compare
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 introduces a significant and valuable feature, allowing users to define multiple Kustomization sets that are applied sequentially. This is a great enhancement for managing complex deployments with dependencies, such as installing CRDs before related applications. The implementation is well-structured, refactoring the logic into new, reusable Terraform modules (user_kustomization_set and user_kustomizations). The main deployment script is particularly robust, with good error handling and cleanup mechanisms. My review includes a couple of suggestions to further improve the implementation: one to optimize the file upload process for better efficiency, and another to enhance the robustness of the pre/post command scripts. Overall, this is an excellent contribution that greatly improves the flexibility of the module.
|
/gemini review |
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 introduces a significant and well-designed feature for sequential Kustomization deployments. The refactoring into new modules (user_kustomization_set, user_kustomizations) is clean and improves maintainability. The implementation is robust, incorporating good practices like trap for cleanup, set -e for error handling, and marking sensitive variables. The backward compatibility is also handled gracefully. My review includes a few suggestions to further improve robustness and maintainability, primarily by enhancing the kustomization file detection logic, simplifying a local variable definition, and updating the documentation to reflect the new recommended practices.
|
/gemini review |
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 introduces a significant and well-designed feature for sequential Kustomization deployments. The refactoring into new modules (user_kustomization_set and user_kustomizations) greatly improves modularity and maintainability. The implementation is robust, with good error handling (set -e), cleanup (trap), and validation. The changes to mark sensitive variables are also a welcome security improvement. My feedback is focused on minor improvements to the documentation and example files to enhance clarity and prevent potential user confusion from copy-pasting. Overall, this is an excellent contribution.
568ab19 to
931bbff
Compare
|
/gemini review |
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 introduces a significant and valuable enhancement by allowing sequential Kustomization deployments via the new user_kustomizations variable. The refactoring into user_kustomization_set and user_kustomizations modules is clean, improves maintainability, and the implementation is robust, with good use of triggers, cleanup traps, and validation. The documentation updates are also comprehensive. I've found a functional issue related to file discovery for kustomization templates that prevents using filenames like kustomization.yml.tpl, and a minor typo in the documentation. Once these are addressed, this will be an excellent addition.
|
@vsalomaki Great stuff man! Let's remove it, yes. But also create a small migration guide please (if not already done). About llms.txt, ideally it would be editted by an coding agent like claude code, that would look at the changes from this PR and edit it accordingly. I can do that no worries. |
|
/gemini review |
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 is an excellent pull request that significantly improves the flexibility of custom deployments by introducing sequential Kustomization sets. The refactoring into new, dedicated modules (user_kustomization_set and user_kustomizations) is clean and greatly enhances maintainability. The addition of robust error handling, cleanup traps, and comprehensive validation for the new variables is very well done. The migration guide and updated examples are also much appreciated. I've found a few minor inconsistencies and a small gap in the documentation examples, which I've detailed in the comments. Overall, this is a fantastic enhancement to the module.
461151d to
83febc1
Compare
|
/gemini review |
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 introduces a significant and well-executed refactoring to allow for sequential Kustomization deployments via the new user_kustomizations variable. The modular approach is clean, robust, and greatly enhances flexibility for complex scenarios like CRD installations. My review includes a few minor documentation tweaks for clarity and a high-priority security enhancement to ensure that pre_commands and post_commands are treated as sensitive data, preventing potential secret leakage in Terraform plans.
|
@mysticaltech I added MIGRATION.md that has information on how to update from the previous version |
|
Thanks @vsalomaki ! |
|
@codex review please, and let me know if backward compatible or not. |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| variable "user_kustomizations" { | ||
| type = map(object({ | ||
| source_folder = string | ||
| kustomize_parameters = optional(map(any), {}) | ||
| pre_commands = optional(string, "") | ||
| post_commands = optional(string, "") | ||
| })) | ||
| default = {} | ||
| description = "All values will be passed to the `kustomization.tmp.yml` template." | ||
| } | ||
| description = "Map of Kustomization-set entries, where key is the order number." | ||
|
|
||
| variable "extra_kustomize_folder" { | ||
| type = string | ||
| default = "extra-manifests" | ||
| description = "Folder from where to upload extra manifests" | ||
| validation { | ||
| condition = alltrue([ | ||
| for key in keys(var.user_kustomizations) : | ||
| can(regex("^[0-9]+$", key)) && tonumber(key) > 0 | ||
| ]) | ||
| error_message = "All keys in user_kustomizations must be positive numeric strings (e.g., '1', '2')." | ||
| } |
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.
Removing extra_kustomize_ variables breaks existing module inputs*
This change deletes the extra_kustomize_deployment_commands, extra_kustomize_parameters and extra_kustomize_folder variables and replaces them with the new user_kustomizations map. Any consumer that still sets the old variables—either through kube.tf, tfvars, or module inputs—will now fail to plan with “An argument named … is not expected here”. Unless the next release is explicitly a breaking major version with a deprecation cycle, this is not backward compatible with 2.18.x users and will block upgrades until they rename variables. Consider preserving the old variables with deprecation warnings or providing a compatibility wrapper.
Useful? React with 👍 / 👎.
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.
The actions to be made are described in MIGRATION.md
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.
Should we go with the MIGRATION.md, @mysticaltech ?
7d7d708 to
69ef912
Compare
94851e7 to
c4c361b
Compare
|
/gemini review |
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 introduces a significant and well-designed refactoring of the user kustomization feature. It replaces the old, limited variables with a flexible user_kustomizations map, allowing for the sequential application of multiple kustomization sets. The implementation is robust, using new modules, proper triggers for updates, and security best practices like marking sensitive variables and ensuring cleanup of temporary files on the remote host. The documentation updates are comprehensive and include a helpful migration guide. My review found a couple of minor issues in the documentation examples that could be improved for clarity and correctness, but overall this is an excellent contribution.
c4c361b to
18196bc
Compare
|
/gemini summary |
Summary of ChangesThis pull request significantly enhances the Kubernetes cluster bootstrapping process by introducing a flexible, sequential Kustomization deployment mechanism. It refactors existing pre- and post-install Kustomization logic into a modular, configurable system, allowing users to define multiple Kustomization steps with specific ordering and post-deployment actions. This change improves maintainability, addresses potential conflicts, and provides greater control over complex application deployments, particularly for scenarios requiring CRD installation before dependent applications. Highlights
Changelog
Activity
|
c78506e to
f744991
Compare
…e used to define multiple Kustomization-sets that are run sequentially. This allows to e.g. install CRD in one step, wait for their deployment and then proceed with the next set.
f744991 to
44717f7
Compare
Description
Adds
user_kustomizations-parameter and supporting modules, which can be used to define multiple Kustomization-sets that are run sequentially. This allows to e.g. install CRD in one step, wait for their deployment and then proceed with the next set.NOTE: Breaking change,
user_kustomizationsreplacesextra_kustomize_deployment_commands,extra_kustomize_parametersandextra_kustomize_folder. SeeMIGRATION.mdKey point from kube.tf.example:
Test plan
extra-manifest-preinstalland apply. Check that the required helm-charts were applied.Gemini