ROX-24172: Added compliance report generator to email report #11177
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11177 +/- ##
==========================================
- Coverage 47.99% 47.97% -0.03%
==========================================
Files 2335 2338 +3
Lines 167192 167357 +165
==========================================
+ Hits 80248 80283 +35
- Misses 80575 80704 +129
- Partials 6369 6370 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ksurabhi91
force-pushed
the
generator
branch
2 times, most recently
from
97e638e
to
48b96f9
Compare
ksurabhi91
changed the title
| @@ -57,6 +58,20 @@ func (c *GenericWriter) WriteBytes(buf *bytes.Buffer) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // WriteBytes writes out csv header and values to the provided buffer | |||
| func (c *GenericWriter) WriteCSV(w io.Writer) error { | |||
There was a problem hiding this comment.
I'm curious, why did you take the approach of adding a new method here instead of following the existing pattern?
There was a problem hiding this comment.
To write csv data in zip io writer and not create a new buffer like vuln reporting
There was a problem hiding this comment.
cc: @charmik-redhat to shed light on why it was done differently for vuln. To get better understanding for myself.
central/complianceoperator/v2/report/manager/reportgenerator/consts.go
Outdated
Show resolved
Hide resolved
central/complianceoperator/v2/report/manager/reportgenerator/report_gen_impl.go
Outdated
Show resolved
Hide resolved
central/complianceoperator/v2/report/manager/reportgenerator/report_gen_impl.go
Outdated
Show resolved
Hide resolved
| return rg.sendEmail(zipData, formatEmailBody, formatEmailSub, req.notifiers, ctx) | ||
| } | ||
|
|
||
| func (rg *complianceReportGeneratorImpl) getDataforReport(req *ComplianceReportRequest, ctx context.Context) (*resultEmail, error) { |
There was a problem hiding this comment.
I think we should separate this even further, though that can wait for the follow on work. 1 func to get the overview stats for the body of the email and another to get the actual results of the checks.
There was a problem hiding this comment.
Yes will do that in following prs
| "Mixed:{{.Mixed}} checks \n" + | ||
| "Clusters {{.Clusters}} scanned" | ||
|
|
||
| defaultSubjectTemplate = "{{.BrandedPrefix}} Compliance Report For {{.ScanConfig}} Profiles {{.Profiles}}" |
There was a problem hiding this comment.
Curious if adding the list of profiles might make the subject too long
There was a problem hiding this comment.
I can cap max number of profiles that will be added tot he subject
ksurabhi91
force-pushed
the
generator
branch
2 times, most recently
from
3107a58
to
8323807
Compare
central/complianceoperator/v2/report/manager/complianceReportgenerator/consts.go
Outdated
Show resolved
Hide resolved
central/complianceoperator/v2/report/manager/complianceReportgenerator/report_gen.go
Show resolved
Hide resolved
central/complianceoperator/v2/report/manager/complianceReportgenerator/report_gen_impl.go
Outdated
Show resolved
Hide resolved
central/complianceoperator/v2/report/manager/complianceReportgenerator/report_gen_impl.go
Show resolved
Hide resolved
| } | ||
|
|
||
| type resultEmail struct { | ||
| resultCSVs map[string][]*resultRow //map of cluster id to slice of *resultRow |
There was a problem hiding this comment.
This can get very large, need to vet the scale issues
| Profiles: profiles, | ||
| } | ||
|
|
||
| return rg.sendEmail(zipData, formatEmailBody, formatEmailSub, req.notifiers, ctx) |
There was a problem hiding this comment.
We need to send the email asynchronously and not wait for the send to complete - I think we need to use a go routine in sendEmail to just post it to the notifier and return
There was a problem hiding this comment.
Made sendemail async but dont know how to make sure that ProcessReportRequest does not exit before sendemail routine completes? I can add waitgroups but then that will block manager to call processreportrequest
central/complianceoperator/v2/report/manager/complianceReportgenerator/report_gen_impl.go
Show resolved
Hide resolved
| @@ -57,6 +58,20 @@ func (c *GenericWriter) WriteBytes(buf *bytes.Buffer) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // WriteBytes writes out csv header and values to the provided buffer | |||
| func (c *GenericWriter) WriteCSV(w io.Writer) error { | |||
There was a problem hiding this comment.
cc: @charmik-redhat to shed light on why it was done differently for vuln. To get better understanding for myself.
ksurabhi91
force-pushed
the
generator
branch
6 times, most recently
from
f124c59
to
6afeb83
Compare
| // ProcessReportRequest will generate a csv report and send notification via email to attached scan config notifiers. | ||
| ProcessReportRequest(req *ComplianceReportRequest) error | ||
|
|
||
| getDataForReport(req *ComplianceReportRequest) (*resultEmail, error) |
There was a problem hiding this comment.
It seems odd that we have private funcs in an interface. Should this just be a struct and not an interface?
There was a problem hiding this comment.
I have removed private functions
ksurabhi91
force-pushed
the
generator
branch
3 times, most recently
from
24b401b
to
67cc1f3
Compare
|
@ksurabhi91: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
Images are ready for the commit at ffe9bc3. To use with deploy scripts, first |
This PR adds changes to