-
Notifications
You must be signed in to change notification settings - Fork 776
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
feat: stats in webhook, audit & gator #2686
feat: stats in webhook, audit & gator #2686
Conversation
1ce1c9f
to
e85f1b1
Compare
Signed-off-by: Alex Pana <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
e85f1b1
to
14c792f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2686 +/- ##
==========================================
+ Coverage 52.76% 52.80% +0.04%
==========================================
Files 123 125 +2
Lines 10941 11053 +112
==========================================
+ Hits 5773 5837 +64
- Misses 4711 4759 +48
Partials 457 457
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@maxsmythe PTAL 🙏🏼 |
LGTM with nits (after Max's comments are addressed about the output file flag and skipping self-validation in g8r) We should update the docs too, but this can be done as a follow up if you prefer. |
Signed-off-by: Alex Pana <[email protected]>
92ab0e8
to
f2f1026
Compare
regarding
I was planning on updating the docs separately. |
Signed-off-by: Alex Pana <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
pkg/logging/logging_test.go
Outdated
} | ||
|
||
func (c *fakeCore) Sync() error { | ||
return nil // TODO(acpana): revisit implementation for GC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before we merge, can we open an issue tracking this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -62,6 +62,7 @@ var ( | |||
auditMatchKindOnly = flag.Bool("audit-match-kind-only", false, "only use kinds specified in all constraints for auditing cluster resources. if kind is not specified in any of the constraints, it will audit all resources (same as setting this flag to false)") | |||
apiCacheDir = flag.String("api-cache-dir", defaultAPICacheDir, "The directory where audit from api server cache are stored, defaults to /tmp/audit") | |||
emptyAuditResults []updateListEntry | |||
logStatsAudit = flag.Bool("log-stats-audit", false, "(alpha) log stats metrics for the audit run") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a doc to describe usage? can be a follow-up
) | ||
|
||
func init() { | ||
Cmd.Flags().StringArrayVarP(&flagFilenames, flagNameFilename, "f", []string{}, "a file or directory containing Kubernetes resources. Can be specified multiple times.") | ||
Cmd.Flags().StringVarP(&flagOutput, flagNameOutput, "o", "", fmt.Sprintf("Output format. One of: %s|%s.", stringJSON, stringYAML)) | ||
Cmd.Flags().BoolVarP(&flagIncludeTrace, "trace", "t", false, `include a trace for the underlying constraint framework evaluation`) | ||
Cmd.Flags().BoolVarP(&flagIncludeTrace, "trace", "t", false, "include a trace for the underlying Constraint Framework evaluation.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add a doc that describes usage? can be a follow up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes for stats
feature I was going to do a follow up docs PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few minor comments, otherwise LGTM
Co-authored-by: Sertaç Özercan <[email protected]> Signed-off-by: alex <[email protected]>
Signed-off-by: Alex Pana <[email protected]>
Signed-off-by: Alex Pana <[email protected]> Signed-off-by: alex <[email protected]> Co-authored-by: Sertaç Özercan <[email protected]> Signed-off-by: Anlan Du <[email protected]>
Integrates
stats
under flags foradmission
,audit
&gator test
. This should be viewed as an iterative process and we can revisit the emission mechanism (ie logging) or think about aggregation or prometheus metrics, etc. in follow ups.Docs will come as a follow up once code lands.