-
Notifications
You must be signed in to change notification settings - Fork 4
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 limit and context cancellation handling to current tables. Closes #62 #63
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.
@c0d3r-arnab Please see comments, thanks!
okta_application app | ||
left join okta_app_assigned_group ag on app.id = ag.app_id | ||
left join okta_group grp on ag.id = grp.id; | ||
from |
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.
from | |
from |
@c0d3r-arnab Why do we have extra spaces here?
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 missed it. I have removed it now.
okta/table_okta_trusted_origin.go
Outdated
// Maximum limit isn't mentioned in the documentation | ||
// Default maximum limit is set as 1000 | ||
input := query.Params{ | ||
Limit: 1000, |
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.
What's the reasoning for picking 1000 as the default?
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.
In the other plugins, it was decided to set 1000 as the maximum limit when it is not mentioned in the documentation. Then we will test the query manually and reduce the number if the API doesn't support it. I have followed the same pattern in this case.
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.
It's good to look at other plugins, but we should probably look at Okta's other limits, not other plugins, to base a default off of. What are most defaults for other Okta APIs? 200?
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.
In most of the cases that I have come across so far, it's 200. I have 10000 and 500 in 2 tables.
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.
Let's go with 200 then, it feels like a safe bet based on other APIs - if we come across different information later on, we can update the default.
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 have updated the default limits of both okta_network_zone
and okta_trusted_origin
to 200.
Tables updated:
Example query results
output.txt