refactor: main.go to use cliFlags struct for flag parsing#31
Conversation
…r functions for flag parsing and application setup.
Summary of ChangesHello, 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 significantly enhances the operator's codebase by refactoring several core components to improve code organization, readability, and maintainability. It achieves this by breaking down large, complex functions into smaller, more focused helper methods and encapsulating related logic within dedicated structs or functions. These changes streamline the main execution flow, making the system easier to understand, test, and extend for future development. Highlights
Changelog
Activity
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.
Pull request overview
This PR refactors the operator entrypoint and several controllers/scanners to improve readability by extracting complex logic into focused helper functions, while keeping behavior largely consistent.
Changes:
- Refactors
cmd/main.goto centralize flag parsing into acliFlagsstruct and move setup logic intoparseFlags, TLS/metrics/webhook builders, andsetupControllers. - Modularizes reconciliation logic in
ClusterScanReconciler,SecurityPolicyReconciler, andCostPolicyReconcilerinto smaller helper methods. - Splits scanner logic in
PrivilegeEscalationScannerandPodSecurityScannerinto pod-level/container-level helper checks.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/main.go |
Refactors main into flag parsing + builder helpers and central controller registration. |
internal/controller/clusterscan_controller.go |
Breaks Reconcile into helper phases (finalizer, suspend, execute, reporting). |
internal/controller/securitypolicy_controller.go |
Extracts scanner execution, correlator ingestion, and status updates into helpers. |
internal/controller/costpolicy_controller.go |
Extracts namespace resolution, rightsizing evaluation, and status updates into helpers. |
internal/scanner/privilege_escalation.go |
Splits pod vs container privilege-escalation checks into helper methods. |
internal/scanner/pod_security.go |
Splits host namespace/path/capability/process-sharing checks into helper methods. |
internal/gitops/source/source.go |
Extracts Helm file classification into classifyHelmFiles. |
internal/gitops/discovery/discovery.go |
Splits discovery scanning vs raw-source appending into helper functions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Scan implements Scanner. | ||
| // | ||
| //nolint:gocyclo // Scanner logic evaluates multiple interrelated pod fields | ||
| // Scan implements Scanner. | ||
| func (s *PodSecurityScanner) Scan(_ context.Context, pods []corev1.Pod, _ map[string]string) ([]Finding, error) { |
There was a problem hiding this comment.
The Scan doc comment is duplicated (// Scan implements Scanner. appears twice). This is probably accidental; remove the redundant comment (or reintroduce the previous nolint explanation if you meant to keep a second comment line).
|
|
||
| if err := r.Status().Update(ctx, scan); err != nil { | ||
| return ctrl.Result{}, fmt.Errorf("updating status: %w", err) | ||
| log.Error(err, "Failed to update ClusterScan status") |
There was a problem hiding this comment.
updateFinalStatus logs and continues when the ClusterScan status update fails. That means the reconcile will still emit a "ScanCompleted" event and increment success metrics even though the status write didn't persist, and it also skips controller-runtime retry/backoff behavior. Consider returning the status update error (and only recording completion events/metrics after a successful status update) so failures are retried promptly and metrics remain accurate.
| log.Error(err, "Failed to update ClusterScan status") | |
| log.Error(err, "Failed to update ClusterScan status") | |
| // Record a failed reconcile for visibility and avoid emitting | |
| // successful completion events/metrics when the status update | |
| // itself did not persist. | |
| aotmetrics.ReconcileTotal.WithLabelValues("clusterscan", "error").Inc() | |
| return |
| // Scan implements Scanner. | ||
| // | ||
| //nolint:gocyclo // Container iteration involves multiple contextual checks | ||
| // Scan implements Scanner. |
There was a problem hiding this comment.
The Scan doc comment is duplicated (// Scan implements Scanner. appears twice). This ends up in generated docs and is likely accidental; remove the redundant comment (or restore the prior nolint comment if that was the intent).
| // Scan implements Scanner. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on extensive code refactoring across multiple Go files to improve modularity, readability, and maintainability, specifically targeting functions with high cyclomatic complexity. In cmd/main.go, command-line flag parsing, TLS option building, webhook server creation, metrics options configuration, and controller setup logic have all been extracted into dedicated helper functions and a cliFlags struct. Similarly, in internal/controller/clusterscan_controller.go, costpolicy_controller.go, and securitypolicy_controller.go, the Reconcile methods have been broken down into smaller, more focused helper functions to manage finalizers, suspension, target resolution, scanning, compliance evaluation, report creation, and status updates. The //nolint:gocyclo comments were removed from these refactored functions, indicating a successful reduction in complexity. Further refactoring occurred in internal/gitops/discovery/discovery.go and internal/gitops/source/source.go to modularize file categorization and Helm chart parsing. Finally, internal/scanner/pod_security.go and internal/scanner/privilege_escalation.go saw their Scan methods refactored into specific check functions for host namespaces, paths, capabilities, process sharing, and privilege escalation vectors.
|
Closing — all 8 files have diverged significantly after 6 merged PRs (including v1.0 cloud expansion). The refactoring intent is still valid (all 8 |
This pull request refactors the
cmd/main.goentrypoint and theClusterScanReconcilerto improve code organization, readability, and maintainability. The main function is broken into smaller, purpose-specific functions, and theClusterScanReconcilerlogic is modularized into helper methods for each phase of the reconciliation process. These changes make the codebase easier to understand and extend.Key changes include:
Entrypoint Refactoring (
cmd/main.go):main()function into smaller functions: flag parsing (parseFlags), TLS options construction (buildTLSOpts), webhook and metrics server configuration (buildWebhookServer,buildMetricsOptions), and controller setup (setupControllers). This improves readability and maintainability. [1] [2] [3] [4]cliFlagsstruct to encapsulate all CLI flag values, reducing the number of top-level variables and simplifying flag handling.setupControllersfunction, replacing repeated controller setup blocks with a single call. [1] [2]ClusterScanReconciler Refactoring (
internal/controller/clusterscan_controller.go):Reconcilemethod into smaller helper methods for handling finalizers (handleFinalizer), suspension (handleSuspend), scan execution (executeScan), error handling (handleTargetResolutionError), scanner execution (runScanners), severity counting (incrementSeverityCount), and compliance evaluation (evaluateCompliance). This modularization clarifies the reconciliation flow and makes each concern easier to test and modify. [1] [2] [3] [4] [5] [6]logr.Loggerto support improved logging in helper methods.These changes collectively enhance the maintainability and clarity of the operator's core logic, making it easier for future contributors to understand and extend.