Adopt flux-instance from upstream #363
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes involve a reorganization of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant FluxCD
participant Cluster
User->>FluxCD: Deploy configuration
FluxCD->>Cluster: Apply nested settings
Cluster-->>FluxCD: Configuration applied
FluxCD-->>User: Deployment successful
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 (
|
|
Oh, we used this ^ Setting the events-addr like this negates the impact of a default cluster domain, so we can let the default value be |
|
Hi @kingdonb just been merged, could you please rebase? |
|
So, the impact of this upgrade is: artifact: oci://ghcr.io/controlplaneio-fluxcd/flux-operator-manifests:latest^ This value is added, enabling the automatic update functionality For an idea of what's in there, you can unpack the artifact and inspect it: ... all of the details flux-operator needs to keep Flux up-to-date in conformance with your setting in spec (ours is currently |
Signed-off-by: Kingdon Barrett <kingdon+github@tuesdaystudios.com>
Signed-off-by: Kingdon Barrett <kingdon+github@tuesdaystudios.com>
Signed-off-by: Kingdon Barrett <kingdon+github@tuesdaystudios.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (19)
packages/system/fluxcd/Makefile (1)
9-11: LGTM! Consider updating documentation.The new
updatetarget is a valuable addition for managing the flux-instance chart. It aligns well with the PR objective of adopting flux-instance from upstream.Consider adding a comment above the
updatetarget to explain its purpose and when it should be used. For example:# Update the flux-instance chart from the upstream OCI registry update: rm -rf charts helm pull oci://ghcr.io/controlplaneio-fluxcd/charts/flux-instance --untar --untardir chartsAlso, it might be beneficial to update the README or other documentation to mention this new target and its significance in the workflow.
packages/system/fluxcd/charts/flux-instance/Chart.yaml (1)
10-19: LGTM: Metadata fields are complete and accurate.The general metadata fields are well-structured and provide all necessary information for a Helm chart. The version numbers are consistent, and the kubeVersion constraint is appropriate for modern Kubernetes clusters.
Consider removing the trailing space in the description field on line 12:
-description: 'A Helm chart for deploying a Flux instance managed by Flux Operator. ' +description: 'A Helm chart for deploying a Flux instance managed by Flux Operator.'Also applies to: 27-28
packages/system/fluxcd/charts/flux-instance/templates/instance.yaml (4)
23-25: LGTM with suggestion: Consider adding validation for components, cluster, and kustomize.The use of direct YAML insertion from Helm values for components, cluster, and kustomize specifications provides maximum flexibility. However, consider adding some validation or defaults in the Helm chart to ensure these critical sections are properly configured.
You could add validation in the Helm chart's
values.yamlfile or use Helm's built-in validation features to ensure these sections are properly configured. For example:{{- if not .Values.instance.components }} {{- fail "At least one component must be specified in .Values.instance.components" }} {{- end }}
26-35: LGTM with suggestion: Consider adding validation for sync.kind.The sync specification is well-structured and correctly uses conditional inclusion. However, it might be beneficial to add validation for the
sync.kindvalue to ensure it's set to a valid option.Consider adding validation for the
sync.kindvalue in the Helm chart. For example:{{- if .Values.instance.sync.url }} {{- $validKinds := list "git" "oci" }} {{- if not (has .Values.instance.sync.kind $validKinds) }} {{- fail (printf "Invalid sync.kind: %s. Must be one of: %s" .Values.instance.sync.kind (join ", " $validKinds)) }} {{- end }} sync: kind: {{ .Values.instance.sync.kind }} ... {{- end }}This ensures that only valid kinds (e.g., "git" or "oci") are specified.
36-43: LGTM with suggestion: Consider adding validation for sharding configuration.The storage and sharding specifications are well-structured and correctly use conditional inclusion. However, it might be beneficial to add validation for the
sharding.keyandsharding.shardsvalues to ensure they're set to valid options.Consider adding validation for the sharding configuration in the Helm chart. For example:
{{- if .Values.instance.sharding.shards }} {{- if not .Values.instance.sharding.key }} {{- fail "sharding.key must be set when sharding.shards is specified" }} {{- end }} {{- if lt (len .Values.instance.sharding.shards) 2 }} {{- fail "At least two shards must be specified when using sharding" }} {{- end }} sharding: key: {{ .Values.instance.sharding.key }} shards: {{ .Values.instance.sharding.shards | toYaml | nindent 4 }} {{- end }}This ensures that the sharding key is set when shards are specified and that at least two shards are defined when using sharding.
1-43: Overall assessment: Well-structured and flexible FluxInstance configuration.This new
instance.yamlfile provides a comprehensive and flexible configuration for deploying a Flux instance, aligning well with the PR objectives. The use of Helm templating allows for easy customization, which could help address thecluster.domainissue mentioned in the PR description.While the file doesn't directly implement the
events-addrpatch mentioned in the comments, the flexible structure allows for such customizations to be applied easily. Consider the following suggestions to further improve the configuration:
Implement the
events-addrpatch: You could add a section in the Helm values to allow specifying custom arguments for controllers, and then use these values in the FluxInstance specification. This would provide a clean way to apply the patch mentioned in the comments.Add validation for critical values: As suggested in previous comments, consider adding validation for components, sync kind, and sharding configuration to ensure correct setup.
Document the configuration options: Ensure that the README or values.yaml file clearly explains all available configuration options, especially those related to addressing the
cluster.domainissue and applying patches like theevents-addrone.These enhancements would make the FluxInstance configuration more robust and user-friendly, fully addressing the concerns raised in the PR description and comments.
🧰 Tools
🪛 yamllint
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 9-9: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
packages/system/fluxcd/charts/flux-instance/values.yaml (1)
26-29: Consider adding guidance for storage configuration.The empty storage configuration allows for flexibility, which is good. However, it might be helpful to add comments providing guidance on typical values or use cases. For example:
storage: # @schema required: false # -- Storage class to use for Flux controllers (e.g., "standard", "fast") class: "" # -- Storage size for Flux controllers (e.g., "10Gi") size: ""This would help users understand how to configure storage based on their needs.
packages/system/fluxcd/charts/flux-instance/README.md (3)
14-23: LGTM: Clear installation instructions with a minor suggestionThe installation section provides clear and concise instructions for deploying Flux using Helm. The use of an OCI registry is a modern approach, and the reference to additional documentation is helpful.
Consider adding a note about the
flux-systemnamespace creation:To deploy Flux in the `flux-system` namespace: +> Note: Ensure the `flux-system` namespace exists before running the following command. + ```console helm -n flux-system install flux oci://ghcr.io/controlplaneio-fluxcd/charts/flux-instance--- `33-47`: **LGTM: Comprehensive values table with a minor suggestion** The values table provides a detailed and well-structured overview of the configurable options for the flux-instance chart. The inclusion of links to specific documentation sections for each configuration group is particularly helpful. To address the static analysis hints about bare URLs, consider wrapping the URLs in angle brackets: ```diff -| instance.cluster | object | `{"domain":"cluster.local","multitenant":false,"networkPolicy":true,"tenantDefaultServiceAccount":"default","type":"kubernetes"}` | Cluster https://fluxcd.control-plane.io/operator/fluxinstance/#cluster-configuration | +| instance.cluster | object | `{"domain":"cluster.local","multitenant":false,"networkPolicy":true,"tenantDefaultServiceAccount":"default","type":"kubernetes"}` | Cluster <https://fluxcd.control-plane.io/operator/fluxinstance/#cluster-configuration> |Apply this change to all similar lines in the table (41-46).
🧰 Tools
🪛 Markdownlint
40-40: null
Bare URL used(MD034, no-bare-urls)
41-41: null
Bare URL used(MD034, no-bare-urls)
42-42: null
Bare URL used(MD034, no-bare-urls)
43-43: null
Bare URL used(MD034, no-bare-urls)
44-44: null
Bare URL used(MD034, no-bare-urls)
45-45: null
Bare URL used(MD034, no-bare-urls)
46-46: null
Bare URL used(MD034, no-bare-urls)
49-52: LGTM: Helpful source code links with a minor style suggestionThe inclusion of links to the Flux Operator and charts repositories is valuable for users who want to explore the underlying code.
To address the Markdownlint hint and improve consistency, consider using dashes instead of asterisks for the unordered list:
## Source Code -* <https://github.com/controlplaneio-fluxcd/flux-operator> -* <https://github.com/controlplaneio-fluxcd/charts> +- <https://github.com/controlplaneio-fluxcd/flux-operator> +- <https://github.com/controlplaneio-fluxcd/charts>🧰 Tools
🪛 Markdownlint
51-51: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
52-52: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
packages/system/fluxcd-operator/charts/flux-operator/templates/deployment.yaml (1)
22-27: LGTM! Consider minor formatting adjustment for consistency.The addition of Prometheus annotations is a valuable improvement, enabling metric scraping from the Flux Operator pods. This aligns well with the PR objectives and enhances the monitoring capabilities of the deployment.
For consistency with the rest of the file, consider removing the blank line (line 25) between the new annotations and the existing annotations block. This minor adjustment would maintain a more uniform structure throughout the template.
annotations: prometheus.io/scrape: "true" prometheus.io/port: "8080" prometheus.io/path: "/metrics" - {{- with .Values.commonAnnotations }} {{- toYaml . | nindent 8 }} {{- end }}packages/system/fluxcd-operator/charts/flux-operator/values.yaml (1)
93-98: LGTM! Consider adding a brief comment for clarity.The addition of the
serviceMonitorsection is a good practice for integrating with Prometheus Operator. The default values are sensible, with the ServiceMonitor creation disabled by default and reasonable interval and timeout settings.Consider adding a brief comment above the
serviceMonitorsection to explain its purpose, e.g.:# -- Prometheus Operator ServiceMonitor configuration for scraping metrics from the Flux Operator serviceMonitor: ...This would provide more context for users who might not be familiar with ServiceMonitor resources.
packages/system/fluxcd/charts/flux-instance/values.schema.json (3)
1-14: LGTM! Consider adding descriptions for better documentation.The schema declaration and top-level properties are well-structured and appropriate for a Kubernetes resource configuration. Good choice using JSON Schema Draft 2019-09 for compatibility.
Consider adding
descriptionfields to each property for better self-documentation. For example:"commonAnnotations": { + "description": "Annotations to be added to all resources", "properties": {}, "type": "object" },Also applies to: 148-150
59-115: LGTM! Comprehensive configuration options. Consider adding size validation.The
distribution,kustomize,sharding, andstorageproperties are well-structured and cover important aspects of FluxCD configuration. The flexibility provided, especially in thekustomize.patchesarray, is valuable.Consider adding a pattern validation for the
storage.sizeproperty to ensure it follows Kubernetes resource quantity format:"size": { - "type": "string" + "type": "string", + "pattern": "^([+-]?[0-9.]+)([eEinumkKMGTP]*[-+]?[0-9]*)$" }This would help prevent configuration errors related to invalid storage size specifications.
116-153: LGTM! Well-structured sync configuration and overall schema. Consider adding URL validation.The
syncproperty and the overall schema structure are well-organized and comprehensive, covering all necessary aspects of FluxCD configuration.Consider adding a pattern validation for the
sync.urlproperty to ensure it's a valid URL:"url": { - "type": "string" + "type": "string", + "pattern": "^(https?:\\/\\/)?([\\da-z\\.-]+)\\.([a-z\\.]{2,6})([\\/\\w \\.-]*)*\\/?$" }This would help prevent configuration errors related to invalid repository URLs.
Additionally, great job on making
distributionandclusterrequired properties in theinstanceobject. This ensures that essential configuration is always provided.packages/system/fluxcd-operator/charts/flux-operator/values.schema.json (1)
296-318: LGTM! Consider adding a description for thelabelsproperty.The new
serviceMonitorproperty is well-structured and aligns with the PR objectives. It provides configuration options for Prometheus scraping, which enhances the chart's functionality.Consider adding a description for the
labelsproperty to clarify its purpose. For example:"labels": { + "description": "Additional labels to be added to the ServiceMonitor resource", "properties": {}, "type": "object" },This will provide users with more context when configuring the ServiceMonitor.
packages/system/fluxcd-operator/charts/flux-operator/templates/crds.yaml (3)
80-85: LGTM: New tenantDefaultServiceAccount fieldThe addition of the
tenantDefaultServiceAccountfield enhances the FluxInstance CRD's flexibility for multitenant scenarios. The description is clear and informative.Consider adding an example value in the description to make it even more user-friendly. For instance:
description: |- TenantDefaultServiceAccount is the name of the service account to use as default when the multitenant lockdown is enabled. Defaults to the 'default' service account from the tenant namespace. + Example: "tenant-service-account"
211-217: LGTM: New migrateResources fieldThe addition of the
migrateResourcesfield provides valuable control over the migration process of Flux custom resources. The default value of true ensures backward compatibility and automatic upgrades.To enhance clarity, consider adding a brief note about the implications of setting this to false. For example:
description: |- MigrateResources instructs the controller to migrate the Flux custom resources from the previous version to the latest API version specified in the CRD. Defaults to true. + Setting this to false will prevent automatic migration, which may be desired in certain scenarios.
218-233: LGTM with suggestions: New sharding fieldThe addition of the
shardingfield introduces valuable sharding capabilities to the FluxInstance CRD. The structure and required fields are well-defined.Consider the following improvements:
- Enhance the description of the
shardingfield to explain its purpose and benefits.- Add examples for both
keyandshardsto improve usability.Here's a suggested diff:
description: Sharding holds the specification of the sharding configuration. + It allows for distributing reconciliation workload across multiple controller instances. properties: key: default: sharding.fluxcd.io/key - description: Key is the label key used to shard the resources. + description: Key is the label key used to shard the resources. This label will be used to determine which shard a resource belongs to. + example: "sharding.fluxcd.io/key" type: string shards: description: Shards is the list of shard names. + example: ["shard-1", "shard-2", "shard-3"] items: type: string minItems: 1 type: array required: - shards type: object
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
- packages/system/fluxcd-operator/charts/flux-operator/Chart.yaml (2 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/README.md (2 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/templates/crds.yaml (8 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/templates/deployment.yaml (1 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/templates/servicemonitor.yaml (1 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/values.schema.json (1 hunks)
- packages/system/fluxcd-operator/charts/flux-operator/values.yaml (1 hunks)
- packages/system/fluxcd/Makefile (1 hunks)
- packages/system/fluxcd/charts/flux-instance/.helmignore (1 hunks)
- packages/system/fluxcd/charts/flux-instance/Chart.yaml (1 hunks)
- packages/system/fluxcd/charts/flux-instance/README.md (1 hunks)
- packages/system/fluxcd/charts/flux-instance/templates/NOTES.txt (1 hunks)
- packages/system/fluxcd/charts/flux-instance/templates/_helpers.tpl (1 hunks)
- packages/system/fluxcd/charts/flux-instance/templates/instance.yaml (1 hunks)
- packages/system/fluxcd/charts/flux-instance/values.schema.json (1 hunks)
- packages/system/fluxcd/charts/flux-instance/values.yaml (1 hunks)
- packages/system/fluxcd/templates/flux-instance.yaml (0 hunks)
- packages/system/fluxcd/values.yaml (1 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- packages/system/fluxcd/templates/flux-instance.yaml
✅ Files skipped from review due to trivial changes (2)
- packages/system/fluxcd/charts/flux-instance/.helmignore
- packages/system/fluxcd/charts/flux-instance/templates/NOTES.txt
🧰 Additional context used
🪛 yamllint
packages/system/fluxcd-operator/charts/flux-operator/templates/servicemonitor.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
[warning] 13-13: wrong indentation: expected 2 but found 4
(indentation)
[warning] 14-14: wrong indentation: expected 2 but found 4
(indentation)
packages/system/fluxcd/charts/flux-instance/templates/instance.yaml
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 9-9: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
🪛 Markdownlint
packages/system/fluxcd/charts/flux-instance/README.md
51-51: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
52-52: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
40-40: null
Bare URL used(MD034, no-bare-urls)
41-41: null
Bare URL used(MD034, no-bare-urls)
42-42: null
Bare URL used(MD034, no-bare-urls)
43-43: null
Bare URL used(MD034, no-bare-urls)
44-44: null
Bare URL used(MD034, no-bare-urls)
45-45: null
Bare URL used(MD034, no-bare-urls)
46-46: null
Bare URL used(MD034, no-bare-urls)
🔇 Additional comments (40)
packages/system/fluxcd/Makefile (1)
1-11: Request for clarification on PR objectivesThe Makefile changes improve the package's maintainability and update process. However, it's not immediately clear how these changes relate to the
cluster.domainconfiguration issue mentioned in the PR objectives.Could you please clarify:
- How do these Makefile changes contribute to addressing the
cluster.domainconfiguration issue?- Is the patch mentioned in the PR objectives (for
kustomize-controller,helm-controller, etc.) implemented elsewhere in this PR?- If the patch is not part of this PR, what is the plan to incorporate it?
To assist in verifying the presence of the mentioned patch, you can run the following script:
#!/bin/bash # Description: Search for the patch mentioned in the PR objectives # Test 1: Search for patch files echo "Searching for patch files:" fd -e yaml -e yml | xargs grep -l "kustomize-controller\|helm-controller\|image-reflector-controller\|image-automation-controller" # Test 2: Search for the specific argument addition echo "Searching for the specific argument addition:" rg --type yaml "events-addr=http://notification-controller.cozy-fluxcd.svc"This will help locate any relevant patch files or configurations related to the PR objectives.
packages/system/fluxcd-operator/charts/flux-operator/Chart.yaml (2)
11-11: LGTM: AppVersion update is consistent with version change.The update of
appVersionfrom v0.6.0 to v0.9.0 aligns well with the chart version update. This change likely reflects significant improvements or new features in the Flux Operator.
21-22: LGTM: Maintainers updated to reflect team responsibility.The update to the maintainers section, replacing individual maintainers with the "ControlPlane Flux Team", is a good practice. It centralizes responsibility and uses a team email, which can improve continuity and communication.
packages/system/fluxcd-operator/charts/flux-operator/templates/servicemonitor.yaml (4)
1-3: LGTM: Conditional creation and API version are correct.The conditional creation of the ServiceMonitor resource based on
.Values.serviceMonitor.createis a good practice, allowing users to control whether this resource is created. The API versionmonitoring.coreos.com/v1is correct for ServiceMonitor resources.🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
4-18: LGTM: Metadata section is well-structured and uses Helm templating effectively.The metadata section is correctly implemented:
- Name and namespace are properly set using Helm templating.
- Labels are included from multiple sources (common labels and service monitor-specific labels).
- Annotations are conditionally included based on
.Values.commonAnnotations.Note: The yamllint warnings about indentation can be safely ignored as they are false positives due to Helm templating.
🧰 Tools
🪛 yamllint
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
[warning] 13-13: wrong indentation: expected 2 but found 4
(indentation)
[warning] 14-14: wrong indentation: expected 2 but found 4
(indentation)
19-25: LGTM: Namespace and label selectors are correctly configured.The spec section is well-defined:
- The namespaceSelector correctly matches the release namespace, ensuring the ServiceMonitor only targets resources in the same namespace.
- The selector uses the appropriate
flux-operator.selectorLabelshelper function, which should correctly match the labels of the service being monitored.
1-31: LGTM: Well-structured ServiceMonitor implementation.The overall implementation of the ServiceMonitor resource is well done:
- Conditional creation allows for flexibility in deployment.
- Good use of Helm templating and chart values throughout the file.
- Proper structure following Kubernetes resource definition standards.
Note: The yamllint error "syntax error: expected the node content, but found '-'" can be safely ignored. It's a false positive due to the Helm conditional statement at the beginning of the file.
🧰 Tools
🪛 yamllint
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
[warning] 12-12: wrong indentation: expected 2 but found 4
(indentation)
[warning] 13-13: wrong indentation: expected 2 but found 4
(indentation)
[warning] 14-14: wrong indentation: expected 2 but found 4
(indentation)
packages/system/fluxcd/charts/flux-instance/Chart.yaml (4)
1-9: LGTM: Annotations are well-structured and informative.The annotations section provides valuable metadata for Artifact Hub, including the license (AGPL-3.0) and links to important resources such as documentation, chart source, and the upstream project. This information is crucial for users and adheres to best practices for Helm chart metadata.
20-22: LGTM: Maintainer information is appropriate.The maintainers section correctly lists the ControlPlane Flux Team with a team email address. This approach is good for project continuity and provides a clear point of contact for users and contributors.
24-26: LGTM: Source URLs are comprehensive.The sources section includes links to both the Flux Operator repository and the charts repository. This provides users with easy access to both the core project and the Helm chart source, which is helpful for those who want to explore or contribute to the project.
1-28: Overall, excellent Chart.yaml configuration.This Chart.yaml file is well-structured and comprehensive, adhering to Helm chart best practices. It provides all necessary metadata for deploying a Flux instance managed by the Flux Operator. The annotations, version information, descriptions, and links are all appropriate and informative. The only minor suggestion is to remove a trailing space in the description field.
Great job on creating a clear and informative chart configuration!
packages/system/fluxcd/charts/flux-instance/templates/_helpers.tpl (6)
4-6: LGTM: Well-implemented name helper functionThe
flux-instance.namefunction is correctly implemented. It follows Helm best practices by allowing name overrides and adhering to Kubernetes naming conventions (63-character limit, no trailing hyphens).
13-24: LGTM: Comprehensive fullname helper functionThe
flux-instance.fullnamefunction is well-implemented. It covers all necessary scenarios for generating a fully qualified app name, including handling overrides and constructing names based on release and chart names. The function adheres to Kubernetes naming conventions and Helm best practices.
29-31: LGTM: Properly formatted chart helper functionThe
flux-instance.chartfunction is correctly implemented. It creates a valid chart name and version string suitable for use in Kubernetes labels. The function follows best practices by replacing invalid characters, adhering to length restrictions, and ensuring no trailing hyphens.
36-43: LGTM: Comprehensive common labels helper functionThe
flux-instance.labelsfunction is well-implemented. It generates a comprehensive set of common labels for Kubernetes resources, including chart information, selector labels, version information (when available), and management metadata. This approach follows Kubernetes and Helm best practices for resource labeling.
48-51: LGTM: Concise and effective selector labels helper functionThe
flux-instance.selectorLabelsfunction is correctly implemented. It generates the essential selector labels (app name and instance) required for Kubernetes resources. This approach follows best practices for creating unique and meaningful selectors.
1-51: Excellent implementation of Helm chart helper functionsThis file provides a comprehensive set of helper functions for the Flux instance Helm chart. All functions (
name,fullname,chart,labels, andselectorLabels) are well-implemented, following Helm and Kubernetes best practices. They cover all necessary aspects of resource naming and labeling, ensuring consistency and adherence to Kubernetes conventions throughout the chart.The helpers are designed to be flexible, allowing for overrides where appropriate, and handle edge cases (such as name truncation and character replacement) correctly. This implementation provides a solid foundation for the Flux instance chart and will contribute to maintainable and consistent resource definitions.
packages/system/fluxcd/charts/flux-instance/templates/instance.yaml (2)
1-14: LGTM: Resource definition and metadata section look good.The resource definition and metadata section are well-structured and make good use of Helm templating for flexibility. The indentation warnings from yamllint for lines 8-10 can be safely ignored as they are false positives due to the Helm templating.
🧰 Tools
🪛 yamllint
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 9-9: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
16-22: LGTM: Distribution specification is well-structured.The distribution section is correctly defined and makes good use of Helm values for configuration. The conditional inclusion of the imagePullSecret is a good practice, allowing for flexibility in different deployment scenarios.
packages/system/fluxcd/values.yaml (3)
9-15: Components list looks goodThe list of components under
flux-instance.instance.componentsincludes all the standard controllers for a Flux installation. This configuration appears to be correct and comprehensive.
1-49: Overall configuration looks good and addresses PR objectivesThe restructured
values.yamlfile effectively organizes the Flux configuration under theflux-instance.instancehierarchy. This new structure improves readability and maintainability. The configuration includes all necessary components for a Flux installation and implements kustomize patches that address the cluster domain issue mentioned in the PR objectives.Key points:
- The
cluster.domainconfiguration is commented out, with a note indicating it's overridden in patches.- Kustomize patches add the
--events-addrargument to relevant controllers, implementing the solution you recalled from a previous issue.- Additional optimizations like increased concurrency and adjusted resource limits are included.
This configuration appears to successfully negate the impact of a default cluster domain while maintaining flexibility in the deployment configuration.
1-8: Address thecluster.domainconfiguration issueThe commented out
domainfield in the cluster configuration aligns with the concern mentioned in the PR objectives. The comment suggests that the default value is overridden in patches, which might be related to the solution you're trying to recall.To verify if the
cluster.domainis being set elsewhere, run the following script:#!/bin/bash # Description: Search for cluster.domain configuration in other files # Test: Look for cluster.domain configuration rg --type yaml 'cluster:\s*domain:' -g '!values.yaml' # Test: Check for any domain-related patches rg --type yaml 'patch.*domain' -g '!values.yaml'This will help determine if the
cluster.domainis being set in other files or if there are any domain-related patches that might be affecting the configuration.packages/system/fluxcd/charts/flux-instance/values.yaml (4)
1-4: LGTM: Initial configuration looks good.The
nameOverrideandfullnameOverridefields are set up correctly. The emptynameOverrideallows for flexibility, while thefullnameOverrideis appropriately set to "flux" for the Flux instance.
30-33: LGTM: Sharding configuration is well-structured.The sharding configuration provides a good balance between default values and customization options. The sharding key is appropriately set, and the empty shards list allows users to define their own sharding strategy as needed.
45-49: LGTM: Common annotations and labels are correctly defined.The empty objects for common annotations and labels provide a good starting point, allowing users to add their own custom annotations and labels as needed without any pre-defined defaults that might not be universally applicable.
6-18: Consider specifying a more precise version and reviewing component selection.
The distribution version "2.x" is quite broad. Consider specifying a more precise version range to avoid unexpected updates.
The artifact URL uses the "latest" tag. This can lead to reproducibility issues. It's generally recommended to use specific version tags for production deployments.
The components list doesn't include
image-reflector-controllerandimage-automation-controller, which are mentioned in the schema comment. Verify if these components are intentionally excluded or if they should be added based on your requirements.To check the latest stable version of Flux, you can run:
Consider updating the version to the latest stable release or a specific range (e.g., "~2.1.0" for patch updates only).
packages/system/fluxcd/charts/flux-instance/README.md (4)
1-7: LGTM: Clear and informative headerThe header section provides a concise and informative introduction to the flux-instance chart. The version information is consistent, and the description accurately summarizes the chart's purpose.
9-12: LGTM: Clear prerequisitesThe prerequisites section clearly states the minimum required versions for Kubernetes and Helm. This information is essential for users to ensure compatibility before installation.
25-31: LGTM: Clear uninstallation instructionsThe uninstallation section provides a clear command for removing Flux. The note about not affecting managed resources is crucial information for users considering uninstallation.
1-52: Overall: Excellent documentation for the flux-instance chartThis README.md file provides comprehensive and well-structured documentation for the flux-instance Helm chart. It covers all essential aspects, including prerequisites, installation, uninstallation, configuration options, and source code references. The content aligns well with the PR objectives, offering clear guidance for deploying and managing Flux within a Kubernetes environment.
The minor style suggestions provided earlier will further enhance the consistency and adherence to Markdown best practices. Great job on creating this informative and user-friendly documentation!
🧰 Tools
🪛 Markdownlint
51-51: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
52-52: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
40-40: null
Bare URL used(MD034, no-bare-urls)
41-41: null
Bare URL used(MD034, no-bare-urls)
42-42: null
Bare URL used(MD034, no-bare-urls)
43-43: null
Bare URL used(MD034, no-bare-urls)
44-44: null
Bare URL used(MD034, no-bare-urls)
45-45: null
Bare URL used(MD034, no-bare-urls)
46-46: null
Bare URL used(MD034, no-bare-urls)
packages/system/fluxcd-operator/charts/flux-operator/templates/deployment.yaml (1)
22-24: Verify metrics port configuration and consider updating documentation.The new Prometheus annotations correctly specify port 8080 for metrics scraping, which aligns with the existing
http-metricsport configuration in the container spec. This consistency is good.To ensure that the metrics are indeed served on port 8080 and accessible via the
/metricspath, you may want to verify this in your testing environment.Consider updating the chart's README or values file to document this new monitoring capability. This will help users understand that Prometheus scraping is now configured by default and how they can customize it if needed.
Also applies to: 79-81
packages/system/fluxcd-operator/charts/flux-operator/values.yaml (2)
100-104: LGTM! Marketplace settings placement is appropriate.The Marketplace settings have been moved to follow the new ServiceMonitor section, which is a logical placement in terms of configuration grouping. The content remains unchanged, maintaining consistency with previous versions.
93-104: Summary: Positive enhancement for observabilityThe addition of the ServiceMonitor configuration enhances the chart's integration capabilities with Prometheus Operator, which is a valuable feature for monitoring and observability in Kubernetes environments. The default settings are conservative (disabled by default) and allow for easy customization.
The placement of both the new ServiceMonitor section and the existing Marketplace settings maintains a logical structure in the values file. These changes improve the chart's functionality without disrupting existing configurations, making it a positive enhancement overall.
packages/system/fluxcd-operator/charts/flux-operator/README.md (3)
5-6: Repository URL update is correct and beneficial.The change to the Flux Operator repository URL provides a more accurate link to the specific project repository. This update enhances the documentation's precision and user experience.
52-52: New serviceMonitor configuration looks good. Verify implementation in templates.The addition of the
serviceMonitorconfiguration aligns with the updates for Prometheus scraping mentioned in the PR summary. The default values appear reasonable, withcreateset tofalseas a safe default.Please run the following script to verify the implementation of the
serviceMonitorin the chart's templates:Ensure that the
serviceMonitorresource is properly defined and that its creation is conditional based on the.Values.serviceMonitor.createsetting.✅ Verification successful
serviceMonitor implementation verified successfully.
The
serviceMonitoris correctly defined in the templates and its creation is properly conditional based on the.Values.serviceMonitor.createsetting.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify serviceMonitor implementation in templates # Test: Check if serviceMonitor is implemented in templates grep -r "serviceMonitor" packages/system/fluxcd-operator/charts/flux-operator/templates/ # Test: Verify if conditional creation is implemented grep -r "{{- if .Values.serviceMonitor.create }}" packages/system/fluxcd-operator/charts/flux-operator/templates/Length of output: 863
3-3: Version update looks good. Verify consistency across files.The version update from 0.6.0 to 0.9.0 aligns with the PR objective of adopting from upstream.
Please run the following script to ensure version consistency across relevant files:
packages/system/fluxcd/charts/flux-instance/values.schema.json (1)
44-58: LGTM! Well-structured components configuration.The
componentsproperty is well-defined, ensuring only valid components can be specified and preventing duplicates. This structure provides clear guidance for users configuring their FluxCD instance.packages/system/fluxcd-operator/charts/flux-operator/templates/crds.yaml (3)
6-6: LGTM: Controller-gen version updateThe update of controller-gen.kubebuilder.io/version to v0.16.1 is a good practice to keep dependencies up-to-date. This minor version update likely includes bug fixes and small improvements.
338-339: LGTM: Simplified Condition descriptionThe simplification of the Condition object description in both FluxInstance and FluxReport CRDs improves readability while retaining essential information. This change makes the CRD more concise and easier to understand.
Also applies to: 640-641
Line range hint
1-700: Overall assessment: Excellent improvements to FluxInstance and FluxReport CRDsThe changes in this file significantly enhance the functionality and usability of the FluxInstance and FluxReport CustomResourceDefinitions. Key improvements include:
- Updated controller-gen version for potential bug fixes and improvements.
- New
tenantDefaultServiceAccountfield for better multitenancy support.- New
migrateResourcesfield for controlled migration of Flux custom resources.- Introduction of sharding capabilities with the new
shardingfield.- Simplified descriptions for better readability.
These changes demonstrate a commitment to improving the Flux operator's flexibility and feature set. The minor suggestions provided in the review comments can further enhance the usability and clarity of the CRDs.
| NAME=fluxcd | ||
| NAMESPACE=cozy-$(NAME) | ||
|
|
||
| include ../../../scripts/package.mk |
There was a problem hiding this comment.
💡 Codebase verification
Missing Included Script: package.mk
The Makefile includes ../../../scripts/package.mk, but the script does not exist at the specified path. This inclusion will lead to build failures or unexpected behavior.
- Action Required:
- Verify the correct path for
package.mk. - Ensure that
package.mkis added to the repository if it is intended to be used.
- Verify the correct path for
🔗 Analysis chain
LGTM! Verify the included script.
The inclusion of package.mk is a good practice for sharing common Makefile configurations. This change improves consistency across packages.
Please run the following script to verify the existence and content of the included script:
Verification Needed for Included Script
It seems the previous script did not verify the existence and content of package.mk as intended. Please run the following script to ensure that package.mk exists and to display its content:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and content of package.mk
# Test 1: Check if the file exists
if [ -f "../../../scripts/package.mk" ]; then
echo "package.mk exists."
# Test 2: Display the content of package.mk
cat "../../../scripts/package.mk"
else
echo "Error: package.mk does not exist at the specified path."
fi
Length of output: 772
Script:
#!/bin/bash
# Description: Verify the existence and content of package.mk
# Test 1: Check if the file exists
if [ -f "../../../scripts/package.mk" ]; then
echo "package.mk exists."
# Test 2: Display the content of package.mk
cat "../../../scripts/package.mk"
else
echo "Error: package.mk does not exist at the specified path."
fi
Length of output: 161
| - https://github.com/controlplaneio-fluxcd/charts | ||
| type: application | ||
| version: 0.6.0 | ||
| version: 0.9.0 |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Update CHANGELOG for version 0.9.0
The CHANGELOG.md files do not include an entry for version 0.9.0. Please add the necessary details to document the changes introduced in this version.
🔗 Analysis chain
LGTM: Version update is consistent with appVersion change.
The update of version from 0.6.0 to 0.9.0 aligns well with the appVersion update. This significant version bump likely indicates substantial changes or improvements in the Flux Operator.
To ensure all changes are properly documented, please run the following script to check for a CHANGELOG update:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the CHANGELOG has been updated to reflect the new version.
# Test: Search for the new version number in the CHANGELOG. Expect: At least one occurrence.
if rg --type md -q "## \[0\.9\.0\]" $(fd -t f "CHANGELOG\.md"); then
echo "CHANGELOG appears to be updated with version 0.9.0"
else
echo "WARNING: CHANGELOG may not be updated with version 0.9.0"
fi
Length of output: 589
| endpoints: | ||
| - targetPort: 8080 | ||
| path: /metrics | ||
| interval: {{ .Values.serviceMonitor.interval }} | ||
| scrapeTimeout: {{ .Values.serviceMonitor.scrapeTimeout }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making targetPort and path configurable.
The endpoints configuration looks good overall, but there's room for improvement:
- The
targetPortis hardcoded to 8080. Consider making this configurable via.Values.serviceMonitor.targetPort. - The metrics
pathis hardcoded to "/metrics". Consider making this configurable via.Values.serviceMonitor.path.
This would provide more flexibility for users who might need to adjust these values.
Here's a suggested improvement:
endpoints:
- targetPort: {{ .Values.serviceMonitor.targetPort | default 8080 }}
path: {{ .Values.serviceMonitor.path | default "/metrics" }}
interval: {{ .Values.serviceMonitor.interval }}
scrapeTimeout: {{ .Values.serviceMonitor.scrapeTimeout }}Don't forget to update the values.yaml file with these new options and their default values.
| kustomize: | ||
| patches: | ||
| - target: | ||
| kind: Deployment | ||
| name: "(kustomize-controller|helm-controller|source-controller)" | ||
| patch: | | ||
| - op: add | ||
| path: /spec/template/spec/containers/0/args/- | ||
| value: --concurrent=20 | ||
| - op: add | ||
| path: /spec/template/spec/containers/0/args/- | ||
| value: --requeue-dependency=5s | ||
| - op: replace | ||
| path: /spec/template/spec/containers/0/resources/limits | ||
| value: | ||
| cpu: 2000m | ||
| memory: 2048Mi | ||
| - target: | ||
| kind: Deployment | ||
| name: source-controller | ||
| patch: | | ||
| - op: add | ||
| path: /spec/template/spec/containers/0/args/- | ||
| value: --storage-adv-addr=source-controller.cozy-fluxcd.svc | ||
| - op: add | ||
| path: /spec/template/spec/containers/0/args/- | ||
| value: --events-addr=http://notification-controller.cozy-fluxcd.svc/ | ||
| - target: | ||
| kind: Deployment | ||
| name: (kustomize-controller|helm-controller|image-reflector-controller|image-automation-controller) | ||
| patch: | | ||
| - op: add | ||
| path: /spec/template/spec/containers/0/args/- | ||
| value: --events-addr=http://notification-controller.cozy-fluxcd.svc/ |
There was a problem hiding this comment.
💡 Codebase verification
Missing --events-addr Argument in Patches for Some Controllers
The verification reveals that the --events-addr argument is only configured for the source-controller. The following controllers are missing the --events-addr argument:
- kustomize-controller
- helm-controller
- image-reflector-controller
- image-automation-controller
- notification-controller
Please update the packages/system/fluxcd/values.yaml file to include the --events-addr argument for these controllers to align with the PR objectives.
🔗 Analysis chain
Kustomize patches align with PR objectives
The kustomize patches in this configuration appear to implement the solution you mentioned in the PR objectives. Specifically:
- The patches add the
--events-addr=http://notification-controller.cozy-fluxcd.svc/argument to multiple controllers (source, kustomize, helm, image-reflector, and image-automation). - This configuration effectively negates the impact of a default cluster domain, as you described.
The patches also include other optimizations like increasing concurrency and adjusting resource limits, which seem beneficial.
To ensure all necessary controllers are covered by these patches, run the following script:
This script will help ensure that all controllers listed in the components section are also covered by the patches adding the events-addr argument.
The initial verification script encountered errors. Please use the corrected script below to ensure all Flux controllers are properly patched:
Pending Verification of --events-addr Argument in Patches
To ensure that the --events-addr argument is correctly applied to all intended controllers, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all Flux controllers are patched with the events-addr argument
# Test: Check for controllers in the components list
components=$(rg --type yaml '^ components:' -A 10 packages/system/fluxcd/values.yaml | grep -oP '(?<=- ).*-controller')
# Test: Check for controllers in the patches
patched_controllers=$(rg --type yaml 'name:.*controller' packages/system/fluxcd/values.yaml | grep -oP '(?<=name: )\(.*?\)' | tr -d '()')
echo "Components:"
echo "$components"
echo "Patched controllers:"
echo "$patched_controllers"
# Test: Compare the lists
for component in $components; do
if ! echo "$patched_controllers" | grep -q "$component"; then
echo "Warning: $component is not patched with the events-addr argument"
fi
done
Length of output: 744
Script:
#!/bin/bash
# Description: Verify all Flux controllers are patched with the events-addr argument
# Test: Check for controllers in the components list
components=$(rg --type yaml '^ components:' -A 10 packages/system/fluxcd/values.yaml | grep -oP '(?<=- ).*-controller')
# Test: Check for controllers in the patches
patched_controllers=$(rg --type yaml 'name:.*controller' packages/system/fluxcd/values.yaml | grep -oP '(?<=name: )\(.*?\)' | tr -d '()' | tr '|' '\n')
echo "Components:"
echo "$components"
echo "Patched controllers:"
echo "$patched_controllers"
# Test: Compare the lists
for component in $components; do
if ! echo "$patched_controllers" | grep -q "$component"; then
echo "Warning: $component is not patched with the events-addr argument"
fi
done
Length of output: 2368
Script:
#!/bin/bash
# Description: Verify the presence of the --events-addr argument in patches for each controller
# List of controllers to verify
controllers=(
"source-controller"
"kustomize-controller"
"helm-controller"
"image-reflector-controller"
"image-automation-controller"
"notification-controller"
)
echo "Verifying --events-addr argument in patches:"
for controller in "${controllers[@]}"; do
# Search for the --events-addr argument in the patches for the specific controller
events_addr_present=$(rg --type yaml "name: $controller" packages/system/fluxcd/values.yaml -A 10 | grep -- "--events-addr=http://notification-controller.cozy-fluxcd.svc/")
if [ -n "$events_addr_present" ]; then
echo "✔️ $controller has the --events-addr argument configured."
else
echo "❌ $controller is missing the --events-addr argument."
fi
done
Length of output: 2710
| # -- Cluster https://fluxcd.control-plane.io/operator/fluxinstance/#cluster-configuration | ||
| cluster: # @schema required: true | ||
| type: kubernetes # @schema enum:[kubernetes,openshift,aws,azure,gcp] | ||
| domain: "cluster.local" | ||
| networkPolicy: true | ||
| multitenant: false | ||
| tenantDefaultServiceAccount: "default" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the cluster domain configurable.
The cluster configuration looks good overall, but there's a potential issue with the cluster.domain setting. According to the PR objectives, having this always specified might cause problems.
Consider implementing a solution similar to the one mentioned in the PR comments:
- Make the
cluster.domainfield optional or provide a way to disable it. - If it's not set or disabled, you could use a Kustomize patch to set the
--events-addrargument for the relevant controllers. This would allow the default value ofcluster.localto be retained without being utilized.
Here's an example of how you might structure the Kustomize patch:
kustomize:
patches:
- target:
kind: Deployment
name: "(kustomize-controller|helm-controller|image-reflector-controller|image-automation-controller)"
patch: |
- op: add
path: /spec/template/spec/containers/0/args/-
value: --events-addr=http://notification-controller.{your-namespace}.svc/This approach would provide more flexibility in deployment configurations.
| # -- Sync https://fluxcd.control-plane.io/operator/fluxinstance/#sync-configuration | ||
| sync: # @schema required: false | ||
| kind: "GitRepository" # @schema enum:[GitRepository,OCIRepository,Bucket] | ||
| url: "" | ||
| ref: "" | ||
| path: "" | ||
| pullSecret: "" | ||
| kustomize: # @schema required: false | ||
| # -- Kustomize patches https://fluxcd.control-plane.io/operator/fluxinstance/#kustomize-patches | ||
| patches: [] # @schema item: object |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding examples and leveraging kustomize patches.
- The sync configuration provides necessary fields, but consider adding comments with examples for each field to guide users. For instance:
sync:
kind: "GitRepository" # @schema enum:[GitRepository,OCIRepository,Bucket]
url: "" # e.g., "https://github.com/org/repo.git"
ref: "" # e.g., "main" or "tags/v1.0.0"
path: "" # e.g., "./clusters/production"
pullSecret: "" # Name of the pull secret for private repositories- The kustomize patches section is currently empty. This section could be used to implement the solution mentioned in the PR objectives. Consider adding an example patch:
kustomize:
patches:
- target:
kind: Deployment
name: "(kustomize-controller|helm-controller|image-reflector-controller|image-automation-controller)"
patch: |
- op: add
path: /spec/template/spec/containers/0/args/-
value: --events-addr=http://notification-controller.{namespace}.svc/This example shows how to use kustomize patches to add the --events-addr argument, which could help address the cluster domain issue mentioned in the PR objectives.
| "cluster": { | ||
| "properties": { | ||
| "domain": { | ||
| "type": "string" | ||
| }, | ||
| "multitenant": { | ||
| "type": "boolean" | ||
| }, | ||
| "networkPolicy": { | ||
| "type": "boolean" | ||
| }, | ||
| "tenantDefaultServiceAccount": { | ||
| "type": "string" | ||
| }, | ||
| "type": { | ||
| "enum": [ | ||
| "kubernetes", | ||
| "openshift", | ||
| "aws", | ||
| "azure", | ||
| "gcp" | ||
| ], | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "type": "object" | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider expanding cluster type options for greater flexibility.
The cluster property is well-structured and covers important configuration aspects. However, the type property might be too restrictive.
Consider adding an "other" option to the type enum to allow for future or custom cluster types:
"type": {
"enum": [
"kubernetes",
"openshift",
"aws",
"azure",
- "gcp"
+ "gcp",
+ "other"
],
"type": "string"
}This change would provide more flexibility while still maintaining a structured schema.
📝 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.
| "cluster": { | |
| "properties": { | |
| "domain": { | |
| "type": "string" | |
| }, | |
| "multitenant": { | |
| "type": "boolean" | |
| }, | |
| "networkPolicy": { | |
| "type": "boolean" | |
| }, | |
| "tenantDefaultServiceAccount": { | |
| "type": "string" | |
| }, | |
| "type": { | |
| "enum": [ | |
| "kubernetes", | |
| "openshift", | |
| "aws", | |
| "azure", | |
| "gcp" | |
| ], | |
| "type": "string" | |
| } | |
| }, | |
| "type": "object" | |
| }, | |
| "cluster": { | |
| "properties": { | |
| "domain": { | |
| "type": "string" | |
| }, | |
| "multitenant": { | |
| "type": "boolean" | |
| }, | |
| "networkPolicy": { | |
| "type": "boolean" | |
| }, | |
| "tenantDefaultServiceAccount": { | |
| "type": "string" | |
| }, | |
| "type": { | |
| "enum": [ | |
| "kubernetes", | |
| "openshift", | |
| "aws", | |
| "azure", | |
| "gcp", | |
| "other" | |
| ], | |
| "type": "string" | |
| } | |
| }, | |
| "type": "object" | |
| }, |
60d99b1 to
a3f3817
Compare
|
👍 rebased - the Flux 2.4 release should land probably on Monday next week, so we'll be ready for that |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/system/fluxcd/charts/flux-instance/templates/instance.yaml (2)
23-25: LGTM with suggestion: Consider adding schema validation for complex structures.The use of
toYamlfor components, cluster, and kustomize sections allows for flexible configuration. However, it may be beneficial to add schema validation to ensure the provided values match the expected structure.Consider using a JSON Schema to validate the values for these complex structures. This can be implemented using a custom Helm template function or a pre-install hook.
36-43: LGTM with suggestion: Consider validating sharding configuration.The storage and sharding sections are well-implemented with conditional inclusion. This approach ensures that these optional configurations are only present when needed.
For the sharding section, consider adding validation to ensure that when shards are specified, the key is also provided. This could be done using a Helm template function:
{{- if .Values.instance.sharding.shards }} sharding: {{- if not .Values.instance.sharding.key }} {{- fail "Sharding key must be specified when shards are defined" }} {{- end }} key: {{ .Values.instance.sharding.key }} shards: {{ .Values.instance.sharding.shards | toYaml | nindent 4 }} {{- end }}This will provide clearer error messages if the configuration is incomplete.
packages/system/fluxcd/charts/flux-instance/README.md (2)
9-23: LGTM: Clear prerequisites and installation instructionsThe prerequisites and installation instructions are well-documented and easy to follow. The reference to further documentation for configuration options is helpful.
Consider adding a note about the importance of having the Flux Operator already installed in the cluster before using this chart. This could prevent potential confusion for new users.
49-52: LGTM: Useful source code links providedThe inclusion of source code links for both the Flux Operator and the charts repository is helpful for users who want to explore the underlying code or contribute to the project.
Consider using dashes instead of asterisks for the unordered list to maintain consistency with Markdown style guidelines:
## Source Code - https://github.com/controlplaneio-fluxcd/flux-operator - https://github.com/controlplaneio-fluxcd/charts🧰 Tools
🪛 Markdownlint
51-51: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
52-52: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- packages/system/fluxcd/Makefile (1 hunks)
- packages/system/fluxcd/charts/flux-instance/.helmignore (1 hunks)
- packages/system/fluxcd/charts/flux-instance/Chart.yaml (1 hunks)
- packages/system/fluxcd/charts/flux-instance/README.md (1 hunks)
- packages/system/fluxcd/charts/flux-instance/templates/NOTES.txt (1 hunks)
- packages/system/fluxcd/charts/flux-instance/templates/_helpers.tpl (1 hunks)
- packages/system/fluxcd/charts/flux-instance/templates/instance.yaml (1 hunks)
- packages/system/fluxcd/charts/flux-instance/values.schema.json (1 hunks)
- packages/system/fluxcd/charts/flux-instance/values.yaml (1 hunks)
- packages/system/fluxcd/templates/flux-instance.yaml (0 hunks)
- packages/system/fluxcd/values.yaml (1 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- packages/system/fluxcd/templates/flux-instance.yaml
✅ Files skipped from review due to trivial changes (1)
- packages/system/fluxcd/charts/flux-instance/templates/NOTES.txt
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/system/fluxcd/Makefile
- packages/system/fluxcd/charts/flux-instance/.helmignore
- packages/system/fluxcd/charts/flux-instance/Chart.yaml
- packages/system/fluxcd/charts/flux-instance/templates/_helpers.tpl
- packages/system/fluxcd/charts/flux-instance/values.schema.json
- packages/system/fluxcd/charts/flux-instance/values.yaml
- packages/system/fluxcd/values.yaml
🧰 Additional context used
🪛 Markdownlint
packages/system/fluxcd/charts/flux-instance/README.md
51-51: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
52-52: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
40-40: null
Bare URL used(MD034, no-bare-urls)
41-41: null
Bare URL used(MD034, no-bare-urls)
42-42: null
Bare URL used(MD034, no-bare-urls)
43-43: null
Bare URL used(MD034, no-bare-urls)
44-44: null
Bare URL used(MD034, no-bare-urls)
45-45: null
Bare URL used(MD034, no-bare-urls)
46-46: null
Bare URL used(MD034, no-bare-urls)
🪛 yamllint
packages/system/fluxcd/charts/flux-instance/templates/instance.yaml
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 9-9: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (7)
packages/system/fluxcd/charts/flux-instance/templates/instance.yaml (3)
1-14: LGTM: Metadata section is well-structured and uses Helm templating effectively.The metadata section is correctly defined and makes good use of Helm templating for dynamic configuration. This approach allows for flexible and reusable chart definitions.
Note: The yamllint warnings about indentation and syntax errors in this section can be safely ignored. They are false positives due to the tool's inability to properly parse Helm template directives.
🧰 Tools
🪛 yamllint
[warning] 8-8: wrong indentation: expected 2 but found 4
(indentation)
[warning] 9-9: wrong indentation: expected 2 but found 4
(indentation)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
15-22: LGTM: Distribution section is well-defined and flexible.The distribution section is correctly structured and uses Helm values for customization. The conditional block for
imagePullSecretis a good practice, allowing users to optionally specify this sensitive information.
26-35: LGTM: Sync section is well-structured and conditionally included.The sync section is correctly implemented with conditional inclusion based on the presence of a URL. This approach allows users to optionally configure sync settings without leaving empty blocks in the resulting YAML when not needed.
packages/system/fluxcd/charts/flux-instance/README.md (4)
1-8: LGTM: Clear and informative header and introductionThe header provides essential information about the chart, including version and type. The introduction succinctly explains the purpose of the chart and its relationship to the Flux Operator.
25-31: LGTM: Clear uninstallation instructionsThe uninstallation instructions are concise and include an important note about not affecting the resources managed by Flux. This information is crucial for users who may want to remove Flux while keeping their deployed resources intact.
33-47: LGTM: Comprehensive values table with clear documentation linksThe values table provides a detailed overview of all configurable options, including types, defaults, and descriptions. The inclusion of links to specific sections of the Flux Instance documentation for each configuration group is particularly helpful for users.
I'd like to highlight that the
instance.distribution.artifactvalue (line 42) aligns with the PR objectives, which mentioned adding this value to enable automatic updates:artifact: "oci://ghcr.io/controlplaneio-fluxcd/flux-operator-manifests:latest"This configuration enables the automatic update functionality as discussed in the PR comments.
🧰 Tools
🪛 Markdownlint
40-40: null
Bare URL used(MD034, no-bare-urls)
41-41: null
Bare URL used(MD034, no-bare-urls)
42-42: null
Bare URL used(MD034, no-bare-urls)
43-43: null
Bare URL used(MD034, no-bare-urls)
44-44: null
Bare URL used(MD034, no-bare-urls)
45-45: null
Bare URL used(MD034, no-bare-urls)
46-46: null
Bare URL used(MD034, no-bare-urls)
1-52: Overall: Excellent documentation for the flux-instance chartThis README.md file provides comprehensive and well-structured documentation for the flux-instance Helm chart. It covers all necessary aspects, including prerequisites, installation, uninstallation, configuration options, and source code links. The content aligns well with the PR objectives, particularly in including the artifact configuration for automatic updates.
The file will serve as a valuable resource for users deploying and managing Flux instances using this chart. The minor style suggestions mentioned earlier are the only improvements to consider.
Great job on creating clear and informative documentation!
🧰 Tools
🪛 Markdownlint
51-51: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
52-52: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
40-40: null
Bare URL used(MD034, no-bare-urls)
41-41: null
Bare URL used(MD034, no-bare-urls)
42-42: null
Bare URL used(MD034, no-bare-urls)
43-43: null
Bare URL used(MD034, no-bare-urls)
44-44: null
Bare URL used(MD034, no-bare-urls)
45-45: null
Bare URL used(MD034, no-bare-urls)
46-46: null
Bare URL used(MD034, no-bare-urls)
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Thank you! |
Builds on #362
The main issue we will have to solve (maybe with a patch) is that
cluster.domainis always specified in this chart;I'm reading to try to recall how we solved this last time.
Summary by CodeRabbit
Release Notes
New Features
ServiceMonitorresource for Prometheus integration.serviceMonitorconfiguration option with default values for scraping settings.FluxInstanceresource configuration file added for deploying a Flux instance.flux-instanceobject.Documentation
NOTES.txtfile directing users to Flux CD operator documentation.Bug Fixes
Chores
.helmignorefile to streamline package building.