-
Notifications
You must be signed in to change notification settings - Fork 240
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 support for fetching Audit Records #394
Comments
Hi there, can I work on this issue? |
@t-junjie that would be great, I appreciate it! I do have a few suggestions which may be helpful for when you do get to implementing it. The first is that for new methods we're adding to the project, we're not creating new The second is that for these types of paginated API calls, for the future we're looking to have two separate methods. The first, I also think we should be moving to a mode where we allow consumers to provide an So the result is I think we'd end up with two new methods: func (c *Client) ListAuditRecords(ctx context.Context, o ListAuditRecordsOptions) (ListAuditRecordsResponse, error) {}
func (c *Client) ListAuditRecordsPaginated(ctx context.Context, o ListAuditRecordOptions, include func(AuditRecord) bool) ([]AuditRecord, error) {} I thought this information might be more helpful to share now instead of during a PR review. Please let me know if you have any questions. |
I forgot to include this above, here is an example of another Lines 156 to 185 in 2f646c3
|
Thanks for the suggestions and example! |
I took a look at the example in #296, as well as the above example. With reference to the following code, would option 2 be more of what you were thinking for the Or would it be similar to option 1, but we would have to convert func (c *Client) ListAuditRecordsPaginated(ctx context.Context, o ListAuditRecordsOptions, include func(AuditRecord) bool) ([]AuditRecord, error) {
v, err := query.Values(o)
if err != nil {
return nil, err
}
if include == nil {
include = func(AuditRecord) bool { return true }
}
var auditRecords []AuditRecord
// Option 1
// if err := c.pagedGet(ctx,"/audit/records?"+v.Encode(),include); err != nil {
// // this does not work because include is not a responseHandler
// }
// Option 2
// resp, err := c.get(ctx, "/audit/records?"+v.Encode())
// if err != nil {
// return nil, err
// }
// var auditResponse ListAuditRecordsResponse
// if err = c.decodeJSON(resp, &auditResponse); err != nil {
// return nil, err
// }
// records := auditResponse.Records
// for _, r := range records {
// if include(r) {
// auditRecords = append(auditRecords, r)
// }
// }
return auditRecords, nil
} |
This is mostly pseudocode, so the func (c *Client) ListAuditRecordsPaginated(ctx context.Context, o ListAuditRecordsOptions, include func(AuditRecord) bool) ([]AuditRecord, error) {
if include == nil {
include = func(AuditRecord) bool { return true }
}
v, err := query.Values(o)
if err != nil {
return nil, err
}
var records []AuditRecord
responseHandler := func(response *http.Response) (APIListObject, error) {
var result ListAuditRecordsResponse
if err := c.decodeJSON(response, &result); err != nil {
return APIListObject{}, err
}
for _, r := range result.Records {
if include(r) {
records = append(records, r)
}
}
return APIListObject{
More: result.More,
Offset: result.Offset,
Limit: result.Limit,
}, nil
}
if err := c.pagedGet(ctx, "/THEACTUALPATH?"+v.Encode(), responseHandler); err != nil {
return nil, err
}
return records, nil
} |
I just tried implementing this and realised that the style of pagination supported has changed. ( The current implementation of APIListObject contains the field Should we change the implementation of APIListObject to use the new cursor-based fields, or create a separate struct e.g. APIListObjectCursorBased for cursor-based pagination? |
When creating this issue I was unaware of this endpoint using the cursor pagination, so I was under the impression it would mostly be a copy/paste of other paginated calls. I was wrong. Sorry about that. 😅 Let me take a look at this a bit to see how to best handle this type of pagination. I assume it would be to create something similar to |
So I think we need to create something similar to type cursor struct {
Limit uint
NextCursor string
}
type cursorHandler func(r *http.Response) (cursor, error)
func (c *Client) cursorGet(ctx context.Context, basePath string, handler cursorHandler) error {
var next string
basePrefix := getBasePrefix(basePath)
for {
var cs string
if len(next) > 0 {
cs = fmt.Sprintf("cursor=%s", next)
}
resp, err := c.do(ctx, http.MethodGet, fmt.Sprintf("%s%s", basePrefix, cs), nil, nil)
if err != nil {
return err
}
c, err := handler(resp)
if err != nil {
return err
}
if len(c.NextCursor) == 0 {
break
}
next = c.NextCursor
}
return nil
} Then I think the actual paginated method call, based roughly on the one I linked above, you'd do something like: handler := func(r *http.Response) (cursor, error) {
var res ListAuditRecordsResponse
if err := c.decodeJSON(r.Body, &res); err != nil {
return cursor{}, err
}
for _, ar := range res.Records {
if include(ar) {
auditRecords = append(auditRecords, ar)
}
}
return cursor{
Limit: res.Limit,
NextCursor: res.NextCursor,
}, nil
}
if err := c.cursorGet(ctx, "/audit/records?"+queryParams.Encode(), handler); err != nil {
return nil, err
}
return auditRecords, nil We'd also need to add a Does that make sense? |
Yes, that does make sense. I am trying to write tests for the Strangely enough, my editor shows that pagedGet has test coverage so I am confused as to whether the tests are actually there. Could you point me to where the tests are written, if they have been written? I will look into writing tests for |
Also, go test -v -run TestClient_Do
=== RUN TestClient_Do
--- FAIL: TestClient_Do (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered] |
#408 was merged. Thank you for helping out with this! |
https://developer.pagerduty.com/api-reference/b3A6Mjc0ODExNA-list-audit-records
The text was updated successfully, but these errors were encountered: