Add azuread_conditional_access_named_location table #191
Conversation
|
Hi @TheRealHouseMouse, thank you for raising the PR. Could you please address the linting issues before we proceed? |
|
@TheRealHouseMouse could you also add the missing table docs as well? Thanks!! |
|
Fixed the linting errors and added table docs 👍 |
There was a problem hiding this comment.
Hi @TheRealHouseMouse, I have left a few review comments. Please have a look.
Additionally, It would be great if we could add more example queries in this table doc
Thanks!
| Get: &plugin.GetConfig{ | ||
| Hydrate: getAdConditionalAccessNamedLocation, | ||
| IgnoreConfig: &plugin.IgnoreConfig{ | ||
| ShouldIgnoreErrorFunc: isIgnorableErrorPredicate([]string{"Request_ResourceNotFound", "Invalid object identifier"}), |
There was a problem hiding this comment.
Should we treat the Invalid object identifier error as a "not found" error? It appears that this error occurs when the input provided is incorrect. In my opinion, it would be better to notify the user to provide the correct input instead.
There was a problem hiding this comment.
It is the same error as in:
table_azuread_conditional_access_policy
table_azuread_user
table_azuread_group
And other tables in the plugin...
I can change them all to "Request_ResourceInvalidIdentifier" but I think it is better to keep it that way and not change the other tables.
| List: &plugin.ListConfig{ | ||
| Hydrate: listAdConditionalAccessNamedLocations, | ||
| IgnoreConfig: &plugin.IgnoreConfig{ | ||
| ShouldIgnoreErrorFunc: isIgnorableErrorPredicate([]string{"Request_UnsupportedQuery"}), |
There was a problem hiding this comment.
I believe we are constructing the query parameter using the key qualifiers provided as query parameters. Are there any specific query parameter combinations where we encounter the Request_UnsupportedQuery error?
Additionally, I noticed that we only build the query parameter when the display_name is included in the WHERE clause.
Instead of adding this to the ignore configuration, it would be better if we could handle it directly in our table code by building the query parameter based on the API's behavior.
There was a problem hiding this comment.
I'm sorry I don't think I fully understand you. I copied this part from the other tables in the plugin. I changed the type column to location_type because type is a reserved in SQL. I can't think about a specific query parameter combinations where we encounter the Request_UnsupportedQuery.
Added support for id, location_type to be handled by our table code. Is it alright now?
| case *models.IpNamedLocation: | ||
| d.StreamListItem(ctx, &ADIpNamedLocationInfo{t}) | ||
| case *models.CountryNamedLocation: | ||
| d.StreamListItem(ctx, &ADCountryNamedLocationInfo{t}) |
There was a problem hiding this comment.
It would be ideal if we could use StreamList to structure the items in a generic structure rather than basing StreamList results on the response type.
This approach would be beneficial if this table is used as a parent for other tables in the future when retrieving parent table data.
For reference:
- StreamList: StreamList Example
- API response manipulation: API Response Manipulation Example
There was a problem hiding this comment.
I managed to change it, but tool me a lot of time and the results isn't elegant at all. I'm not sure at all this is better than before. If you have a suggestion how to make it better please tell me. I think it might be better before this change.
…d of using response type, changed type column to location_type
misraved
changed the title
ParthaI
changed the base branch from
main
to
add-tabel-azuread-conditional-access-named-location
ParthaI
merged commit 17fdaaf
into
turbot:add-tabel-azuread-conditional-access-named-location
Co-authored-by: TheRealHouseMouse <[email protected]> Co-authored-by: anon4mouse <[email protected]>
Example query results
Resolving issue - #190
Results