Skip to content

Conversation

rouke-broersma
Copy link
Contributor

When using ArgoCD in a full gitops context, revision history limit clutters the UI with unused ReplicaSets. Allowing configuration of the revision history limit allows users to set this to a different value, such as 0. With a revision history limit of 0, Kubernetes will auto-remove any ReplicaSet other than the current active ReplicaSet. I have set a default of 10 so the change is backwards compatible, this is the Kubernetes default.

…f 10

Signed-off-by: Rouke Broersma <mobrockers@gmail.com>
@djeebus
Copy link
Collaborator

djeebus commented Feb 8, 2025

Mind bumping the chart version to 0.5.4?

@rouke-broersma
Copy link
Contributor Author

Mind bumping the chart version to 0.5.4?

Of course, I wasn't sure from the contribution guide if I should or not

Signed-off-by: Rouke Broersma <mobrockers@gmail.com>
@zapier-sre-bot
Copy link
Collaborator

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of charts/kubechecks/Chart.yaml

@@ -1,7 +1,7 @@
 apiVersion: v2
 name: kubechecks
 description: A Helm chart for kubechecks
-version: 0.5.3
+version: 0.5.4
 type: application
 maintainers:
   - name: zapier

Feedback & Suggestions: The version bump from 0.5.3 to 0.5.4 is appropriate if there are backward-compatible bug fixes or minor improvements. Ensure that the changes in the codebase reflect this version update. If there are significant changes or new features, consider updating the minor version instead. 📈


😼 Mergecat review of charts/kubechecks/values.yaml

@@ -40,6 +40,7 @@ deployment:
       memory: 256Mi
       cpu: 200m
 
+  revisionHistoryLimit: 10
   replicaCount: 1
 
   image:

Feedback & Suggestions:

  1. Revision History Limit: Adding revisionHistoryLimit: 10 is a good practice to limit the number of old ReplicaSets retained by the deployment. This helps in managing the storage used by old ReplicaSets. However, ensure that the limit set aligns with your rollback strategy and storage capacity. If you anticipate needing more than 10 revisions for rollback purposes, consider increasing this number.

  2. Documentation: It would be beneficial to add a comment explaining the purpose of revisionHistoryLimit for future maintainers. This can help in understanding why this specific limit was chosen.

  3. Consistency Check: Ensure that this change is consistently applied across all similar configurations if applicable, to maintain uniformity in deployment strategies.


😼 Mergecat review of charts/kubechecks/values.schema.json

@@ -93,6 +93,9 @@
         "readinessProbe": {
           "type": "object"
         },
+        "revisionHistoryLimit": {
+          "type": "integer"
+        },
         "replicaCount": {
           "type": "integer"
         },

Feedback & Suggestions:

  1. Validation Enhancement: Consider adding a minimum constraint for the revisionHistoryLimit property to ensure that it is a non-negative integer. This will prevent potential errors from negative values, which are not valid for this field in Kubernetes. For example:

    "revisionHistoryLimit": {
      "type": "integer",
      "minimum": 0
    }
  2. Documentation: Ensure that the addition of revisionHistoryLimit is documented in any relevant documentation or comments. This will help maintainers and users understand the purpose and usage of this new property.

  3. Testing: If there are any automated tests or validation scripts, consider adding test cases to cover scenarios involving the revisionHistoryLimit to ensure it behaves as expected.


😼 Mergecat review of charts/kubechecks/templates/deployment.yaml

@@ -7,6 +7,7 @@ metadata:
   {{- end}}
   labels: {{- include "kubechecks.labels" . | nindent 4 }}
 spec:
+  revisionHistoryLimit: {{ .Values.deployment.revisionHistoryLimit }}
   replicas: {{ .Values.deployment.replicaCount }}
   selector:
     matchLabels:

Feedback & Suggestions:

  1. Default Value for revisionHistoryLimit: Ensure that a sensible default value is set for revisionHistoryLimit in your values.yaml file. This will prevent potential issues if the value is not explicitly set by the user. A common default is 10.

  2. Validation: Consider adding validation to ensure that revisionHistoryLimit is a non-negative integer. This can help prevent deployment errors due to invalid configurations.

  3. Documentation: Update your documentation to include information about the new revisionHistoryLimit parameter. This will help users understand its purpose and how to configure it.

  4. Testing: Ensure that you have tests in place to verify the behavior of the deployment when revisionHistoryLimit is set to different values, including edge cases like 0 and very high numbers.



Dependency Review

Click to read mergecats review!

No suggestions found

@rouke-broersma
Copy link
Contributor Author

rouke-broersma commented Feb 8, 2025

I bumped the chart to 0.5.4, but since this is a new feature for the chart should it not be 0.6.0?

@djeebus djeebus merged commit 7c498bb into zapier:main Feb 24, 2025
4 checks passed
@rouke-broersma rouke-broersma deleted the support-revision-history-limit branch February 24, 2025 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants