• X
• Mastodon
• LinkedIn
• Facebook

central/complianceoperator/v2/rules/datastore/datastore.go (1)

32-36: Deprecate the benchmark-filtered API to prevent regression-prone usage.

With both methods available, future callers can accidentally reintroduce the lossy benchmark filter. Please mark GetControlsByRulesAndBenchmarks as deprecated (or remove it once no callers remain) and steer usage to GetControlsByRules.

Suggested interface annotation

- // GetControlsByRulesAndBenchmarks returns controls by a list of rule names group by control, standard and rule name.
+ // Deprecated: GetControlsByRulesAndBenchmarks applies benchmark filtering and may drop valid controls.
+ // Prefer GetControlsByRules.
+ // GetControlsByRulesAndBenchmarks returns controls by a list of rule names group by control, standard and rule name.
  GetControlsByRulesAndBenchmarks(ctx context.Context, ruleNames []string, benchmarkNames []string) ([]*ControlResult, error)

As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@central/complianceoperator/v2/rules/datastore/datastore.go` around lines 32 -
36, Mark the benchmark-filtered API GetControlsByRulesAndBenchmarks as
deprecated and steer usage to GetControlsByRules: add a clear deprecation
comment/docstring above the method declaration in the interface (mention it is
deprecated and to use GetControlsByRules instead), optionally add a //
Deprecated: ... line so static analyzers pick it up, and scan/update callers to
replace uses of GetControlsByRulesAndBenchmarks with GetControlsByRules (or
remove the method entirely once no callers remain); keep the function signature
intact for now to avoid breakage but add the deprecation note and an issue/todo
to remove it later.

central/complianceoperator/v2/report/manager/results/results_aggregator.go (1)

125-131: Replace the 4-value tuple from getProfileInfo with a typed return object.

Line 147 now returns (string, string, string, error), which is easy to mis-order at call sites (Line 125). A small struct improves safety and readability as this path evolves.

Refactor sketch

+type profileInfo struct {
+    displayName string
+    profileName string
+    profileType string
+}
-
-func (g *Aggregator) getProfileInfo(...) (string, string, string, error) {
+func (g *Aggregator) getProfileInfo(...) (*profileInfo, error) {
    ...
-   return profileInfo, profiles[0].GetName(), profileType, nil
+   return &profileInfo{
+       displayName: profileInfo,
+       profileName: profiles[0].GetName(),
+       profileType: profileType,
+   }, nil
 }
 
- profileInfo, profileName, profileType, err := g.getProfileInfo(...)
+ pInfo, err := g.getProfileInfo(...)
  if err != nil { ... }
- row.Profile = profileInfo
- row.ProfileType = profileType
+ row.Profile = pInfo.displayName
+ row.ProfileType = pInfo.profileType
- controlsInfo, err := g.getControlsInfo(ctx, checkResult, profileName)
+ controlsInfo, err := g.getControlsInfo(ctx, checkResult, pInfo.profileName)

As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."

Also applies to: 147-161

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@central/complianceoperator/v2/report/manager/results/results_aggregator.go`
around lines 125 - 131, Change getProfileInfo to return a typed struct instead
of three separate strings and an error (e.g., type ProfileMeta struct { Name
string; Info string; Type string }), update its signature to (ProfileMeta,
error), and update all call sites including the usage in results_aggregator.go
where you currently do profileInfo, profileName, profileType, err :=
g.getProfileInfo(...) to instead receive a single value (meta, err :=
g.getProfileInfo(...)) and then set row.Profile = meta.Info and row.ProfileType
= meta.Type (and any use of profileName -> meta.Name); also update other callers
referenced around lines 147-161 to use the new struct fields. Ensure imports and
any tests are adjusted to the new type.

central/complianceoperator/v2/report/manager/results/results_aggregator_test.go (1)

356-357: Assert the rule-name slice passed into GetControlsByRules.

Using gomock.Any() here means this suite no longer checks that aggregation is actually scoping controls by the resolved rule names. That’s the regression boundary this PR is fixing, so the mock should verify the expected slice instead of only the call count.

Example tightening

 			if tcase.expectedControls != nil {
-				s.ruleDS.EXPECT().GetControlsByRules(gomock.Any(), gomock.Any()).Times(1).Return(tcase.expectedControls())
+				expRules, _ := tcase.expectedRules()
+				expRuleNames := []string{expRules[0].GetName()}
+				s.ruleDS.EXPECT().
+					GetControlsByRules(gomock.Any(), gomock.Eq(expRuleNames)).
+					Times(1).
+					Return(tcase.expectedControls())
 			}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@central/complianceoperator/v2/report/manager/results/results_aggregator_test.go`
around lines 356 - 357, The test currently uses gomock.Any() for the rule-name
slice when setting the ruleDS.EXPECT().GetControlsByRules call, which lets the
test miss verifying that aggregation scopes by resolved rule names; change the
second gomock.Any() to assert the exact slice of rule names from the testcase
(e.g. gomock.Eq(tcase.resolvedRuleNames) or the field that holds the expected
rule-name slice on tcase) so ruleDS.EXPECT().GetControlsByRules(gomock.Any(),
gomock.Eq(tcase.resolvedRuleNames)).Times(1).Return(tcase.expectedControls())
and import/adjust any test fields as needed to provide that expected slice.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes