Skip to content
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 Sign-in,Directory Audit get/list actions and Create/List/Delete for Application Extensions #61

Merged
merged 8 commits into from
Jun 21, 2021

Conversation

MarkDordoy
Copy link
Contributor

@MarkDordoy MarkDordoy commented Jun 17, 2021

This PR extends the models.go file to contain the required structs for SignInLogs, DirectoryAuditLogs and application extensions.

It also has some basic tests which should be able to perform get and list on directory audit and signin logs as well as create, list and delete for application Extensions

@MarkDordoy MarkDordoy changed the title Md/support signin logs Add Support For Sign-in and Directory Audit get/list actions Jun 17, 2021
@MarkDordoy
Copy link
Contributor Author

Tests past locally for me now. However the List SigninLogs can error out (this is dependent on how busy the tenant is and how many sign in logs there are to list).
I tired adding a top filter but it didnt seem to parse this correctly. eg:

func (c *SignInReportsClient) List(ctx context.Context, filter string, top string) (*[]SignInReport, int, error) {
	params := url.Values{}
	if filter != "" {
		params.Add("$filter", filter)
	}
	if top != "" {
		params.Add("$top", top)
	}
...

I think if we could get that working then i could ask the test to always just bring back top 1 so we get a speedy test. @manicminer if you have any thoughts/idea's on this let me know.

@MarkDordoy MarkDordoy changed the title Add Support For Sign-in and Directory Audit get/list actions Add Support For Sign-in,Directory Audit get/list actions and Create/List/Delete for Application Extensions Jun 18, 2021
@manicminer manicminer added the enhancement New feature or request label Jun 19, 2021
@MarkDordoy
Copy link
Contributor Author

MarkDordoy commented Jun 21, 2021

Hey @manicminer do i need to add anything else to get this approved and merged?

Do you have any suggestions in terms of the tests to try and reduce the time and have the call just ask for the top 1?

@manicminer
Copy link
Owner

Hey @MarkDordoy, sorry for the delay in reviewing.

The $top odata argument is its own query param rather than being a filter, so it needs to be added sort of like this:

func (c *SignInReportsClient) List(ctx context.Context, filter string, top int) (*[]SignInReport, int, error) {
	params := url.Values{}
	if filter != "" {
		params.Add("$filter", filter)
	}
	if top > 0 {
		params.Add("$top", fmt.Sprintf("%d", top))
	}

This isn't very wieldy though - there are lots of requests that support $top and I don't think it'd be nice to add another argument to a load of client methods. I'm not entirely happy about the filter string pattern to be honest - this feels like a good time to add more robust odata query handling across the board.

That aside, this PR looks great. I made a few small changes locally - I renamed the AuditLog model to DirectoryAudit to match the API model and added a couple type aliases for user-specified enums. I also took the chance to sort the models. The tests pass for me and with these changes this will be good to merge.

Could you enable upstream contributions for this PR and I'll push my changes, then we can get this merged? Many thanks for the contribution! :)

@MarkDordoy
Copy link
Contributor Author

Hey @manicminer I've selected the "Allow edits by maintainers" button in this PR, i think thats what you need to be able to push commits. Let me know if it doesnt work

@manicminer
Copy link
Owner

Thanks, that did it - I also had your branch name wrong 😅

@manicminer manicminer added this to the v0.18.0 milestone Jun 21, 2021
@manicminer manicminer merged commit 119373f into manicminer:main Jun 21, 2021
manicminer added a commit that referenced this pull request Jun 21, 2021
@MarkDordoy MarkDordoy deleted the md/support-signin-logs branch June 21, 2021 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants