Skip to content
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

monitor all apps by default #324

Merged
merged 1 commit into from
Dec 11, 2024
Merged

monitor all apps by default #324

merged 1 commit into from
Dec 11, 2024

Conversation

djeebus
Copy link
Collaborator

@djeebus djeebus commented Dec 11, 2024

No description provided.

@zapier-sre-bot
Copy link
Collaborator

Mergecat's Review

Click to read mergecats review!

😼 Mergecat review of docs/usage.md

@@ -57,7 +57,7 @@ The full list of supported environment variables is described below:
 |`KUBECHECKS_LOG_LEVEL`|Set the log output level. One of error, warn, info, debug, trace.|`info`|
 |`KUBECHECKS_MAX_CONCURRENCT_CHECKS`|Number of concurrent checks to run.|`32`|
 |`KUBECHECKS_MAX_QUEUE_SIZE`|Size of app diff check queue.|`1024`|
-|`KUBECHECKS_MONITOR_ALL_APPLICATIONS`|Monitor all applications in argocd automatically.|`false`|
+|`KUBECHECKS_MONITOR_ALL_APPLICATIONS`|Monitor all applications in argocd automatically.|`true`|
 |`KUBECHECKS_OPENAI_API_TOKEN`|OpenAI API Token.||
 |`KUBECHECKS_OTEL_COLLECTOR_HOST`|The OpenTelemetry collector host.||
 |`KUBECHECKS_OTEL_COLLECTOR_PORT`|The OpenTelemetry collector port.||

Feedback & Suggestions:

  • The change from false to true for KUBECHECKS_MONITOR_ALL_APPLICATIONS means that by default, all applications in ArgoCD will be monitored automatically. This could have significant performance implications, especially in large environments. Consider whether this change aligns with the typical use case for most users. If not, it might be better to keep the default as false and allow users to opt-in to this behavior.
  • If the change is intentional and beneficial for most users, ensure that the documentation clearly explains the potential impact of this setting being true by default, so users are aware of the implications.

😼 Mergecat review of cmd/controller_cmd.go

@@ -142,7 +142,8 @@ func init() {
 	stringFlag(flags, "webhook-url-base", "The endpoint to listen on for incoming PR/MR event webhooks. For example, 'https://checker.mycompany.com'.")
 	stringFlag(flags, "webhook-url-prefix", "If your application is running behind a proxy that uses path based routing, set this value to match the path prefix. For example, '/hello/world'.")
 	stringFlag(flags, "webhook-secret", "Optional secret key for validating the source of incoming webhooks.")
-	boolFlag(flags, "monitor-all-applications", "Monitor all applications in argocd automatically.")
+	boolFlag(flags, "monitor-all-applications", "Monitor all applications in argocd automatically.",
+		newBoolOpts().withDefault(true))
 	boolFlag(flags, "ensure-webhooks", "Ensure that webhooks are created in repositories referenced by argo.")
 	stringFlag(flags, "repo-refresh-interval", "Interval between static repo refreshes (for schemas and policies).",
 		newStringOpts().withDefault("5m"))

Feedback & Suggestions:

  1. Default Value Clarity: The addition of newBoolOpts().withDefault(true) sets a default value for the monitor-all-applications flag. Ensure that this change is clearly documented, as it alters the default behavior of the application. This can help prevent unexpected behavior for users who rely on the previous default.

  2. Consistency Check: Verify that the newBoolOpts() function is implemented correctly and consistently with other similar functions like newStringOpts(). This ensures that the default value is applied as expected.

  3. Testing: After making this change, it's important to test the application to confirm that the default value is correctly applied and that it doesn't introduce any unintended side effects.

  4. Security Consideration: If this flag impacts security-sensitive operations, ensure that the default value aligns with the desired security posture of the application.

By addressing these points, you can ensure that the change is robust and user-friendly. 🛠️



Dependency Review

Click to read mergecats review!

No suggestions found

Copy link

github-actions bot commented Dec 11, 2024

Temporary image deleted.

@djeebus djeebus merged commit 40efd62 into main Dec 11, 2024
5 checks passed
@djeebus djeebus deleted the default-to-monitoring-all-apps branch December 11, 2024 03:14
abhi-kapoor pushed a commit to abhi-kapoor/kubechecks that referenced this pull request Dec 16, 2024
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