-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add duration/count metrics to PKI issue and revoke flows #13889
Conversation
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!
if role.KeyType == "any" { | ||
return logical.ErrorResponse("role key type \"any\" not allowed for issuing certificates, only signing"), nil | ||
} | ||
|
||
return b.pathIssueSignCert(ctx, req, data, role, false, false) | ||
resp, err := b.pathIssueSignCert(ctx, req, data, role, false, false) |
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.
nit: Simple return here and within pathSignVerbatim.
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.
:nod: This is what happens when you go through several iterations and then don't check your work.
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.
👍 Just need to address that make fmt error CI is reporting.
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.
This looks pretty good to me. Would be cool if there was a test against the metrics here (I'm not sure how to test metrics).
I'll have a look, I don't think we test metrics often outside the metrics system tests, but I'll make sure. |
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.
Looks Good to Me :)
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, one readability nit / possible error, but it's internally used so up to you.
} | ||
if role == nil { | ||
return logical.ErrorResponse(fmt.Sprintf("unknown role: %s", roleName)), nil | ||
if roleMode > noRole { |
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.
I think this would be clearer with a != ;
Would also be cool to throw an error if the roleMode is not one of the possible values.
* Add duration/count metrics to PKI issue and revoke flows * docs, changelog * tidy * last tidy * remove err * Update callsites * Simple returns * Handle the fact that test cases don't have namespaces * Add mount point to the request * fmt * Handle empty mount point, and add it to unit tests * improvement * Turns out sign-verbatim is tricky, it can take a role but doesn't have to * Get around the field schema problem
No description provided.