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

Track the policy data by source group #2181

Closed
wants to merge 1 commit into from

Conversation

joejstuart
Copy link
Member

Only collect the policy data once for each source group. Before this the data was duplicated for each component.

https://issues.redhat.com/browse/EC-1027

Copy link

codecov bot commented Nov 20, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 12 lines in your changes missing coverage. Please review.

Project coverage is 71.18%. Comparing base (d99dec0) to head (44e6b62).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
cmd/validate/input.go 0.00% 10 Missing ⚠️
cmd/validate/image.go 84.61% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2181      +/-   ##
==========================================
- Coverage   71.22%   71.18%   -0.05%     
==========================================
  Files          89       88       -1     
  Lines        7479     7506      +27     
==========================================
+ Hits         5327     5343      +16     
- Misses       2152     2163      +11     
Flag Coverage Δ
generative 71.18% <57.14%> (-0.05%) ⬇️
integration 71.18% <57.14%> (-0.05%) ⬇️
unit 71.18% <57.14%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
internal/image/validate.go 69.53% <100.00%> (+0.40%) ⬆️
internal/output/output.go 88.26% <ø> (ø)
cmd/validate/image.go 91.54% <84.61%> (-0.30%) ⬇️
cmd/validate/input.go 41.36% <0.00%> (-1.35%) ⬇️

... and 10 files with indirect coverage changes

---- 🚨 Try these New Features:

@joejstuart joejstuart force-pushed the dedup2 branch 2 times, most recently from 01a9434 to b63738a Compare November 21, 2024 14:16
Copy link
Member

@zregvart zregvart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm expecting snapshots that need updating (haven't looked at the CI), so UPDATE_SNAPS=1 make ci should help

@@ -130,7 +131,7 @@ func ValidateImage(ctx context.Context, comp app.SnapshotComponent, snap *app.Sn
return nil, err
}
allResults = append(allResults, results...)
out.Data = append(out.Data, data)
out.Data[fmt.Sprintf("%d", idx)] = append(out.Data[fmt.Sprintf("%d", idx)], data)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be a tad nicer

Suggested change
out.Data[fmt.Sprintf("%d", idx)] = append(out.Data[fmt.Sprintf("%d", idx)], data)
key := strconv.FormatInt(idx, 10)
out.Data[key] = append(out.Data[key], data)

@joejstuart joejstuart force-pushed the dedup2 branch 8 times, most recently from 738ed22 to a234a77 Compare November 21, 2024 23:35
@zregvart
Copy link
Member

The snapshot variable substitution is somewhat rudimentary, I think we either need to indent by 2 or decrease the indent by 2, and the underscores should probably reflect that here

@@ -409,12 +412,17 @@ func validateImageCmd(validate imageValidationFunc) *cobra.Command {
e := fmt.Errorf("error validating image %s of component %s: %w", r.component.ContainerImage, r.component.Name, r.err)
allErrors = errors.Join(allErrors, e)
} else {
for key, val := range r.data {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is quite right. This is merging all the data from different source groups.

Consider something like this:

// Renamed from manyData
var evaluatorData []evaluator.Data
dataCollected := false

for i := 0; i < numComponents; i++ {
  r := <-results
  ...
  if !dataCollected {
    evaluatorData = r.data
    dataCollected = true
  }
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or rather:

// Renamed from manyData
var evaluatorData []evaluator.Data

for i := 0; i < numComponents; i++ {
  r := <-results
  ...
  if evaluatorData == nil {
    evaluatorData = r.data
  }
}

Anyways, I think you get the idea.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we want that since the data could differ per source group?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r.data is a list of data.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm I think I see understand what you're doing now. Why use a map where the keys are indexes? Wouldn't it be more natural to keep it as a slice?

Also, running this for loop for every component is unnecessary. It's just copying data around needlessly. Please consider introducing a mechanism so that is only done for the first component.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me think this whole PR could be replaced with

--- a/cmd/validate/image.go
+++ b/cmd/validate/image.go
@@ -410,7 +410,9 @@ func validateImageCmd(validate imageValidationFunc) *cobra.Command {
                                        allErrors = errors.Join(allErrors, e)
                                } else {
                                        components = append(components, r.component)
-                                       manyData = append(manyData, r.data)
+                                       if len(manyData) == 0 {
+                                               manyData = append(manyData, r.data)
+                                       }
                                        manyPolicyInput = append(manyPolicyInput, r.policyInput)
                                }
                        }

@joejstuart joejstuart force-pushed the dedup2 branch 3 times, most recently from 1585fe3 to 3a22118 Compare November 22, 2024 19:20
Only collect the policy data once for each source
group. Before this the data was duplicated for
each component.

https://issues.redhat.com/browse/EC-1027
@joejstuart
Copy link
Member Author

Closing in favor of this #2184

@joejstuart joejstuart closed this Nov 25, 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