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

API Support for Conditional Access Policy #23

Merged
merged 12 commits into from
Apr 11, 2021
Merged

API Support for Conditional Access Policy #23

merged 12 commits into from
Apr 11, 2021

Conversation

liammoat
Copy link
Contributor

@liammoat liammoat commented Apr 1, 2021

This PR will add API support for conditionalAccessPolicy on the Microsoft Graph.

@liammoat
Copy link
Contributor Author

liammoat commented Apr 1, 2021

@manicminer, this is a draft PR to add API support for conditionalAccessPolicy. I'd like to add some tests for the ConditionalAccessPolicyClient. Can you advise if you have a specific tenant that the tests need to run against?

@manicminer
Copy link
Owner

Hi @liammoat, thanks for this PR, this looks great!

For configuring tests, there is a helper at https://github.com/manicminer/hamilton/blob/main/clients/internal/testing.go which reads the auth configuration from environment variables, so it's up to the user to supply a tenant ID (in this case, for a P1 or P2 tenant) and client credentials.

If you can add some tests, I'll take another look and we can look to get this merged :)

@manicminer manicminer added the enhancement New feature or request label Apr 6, 2021
@liammoat liammoat marked this pull request as ready for review April 9, 2021 12:34
Copy link
Owner

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liammoat Thanks for adding the tests. This mostly LGTM, I have a question about the enterprise application used for testing, and I replied to your query on the readonly field issue.

Sorry for the inconvenience but I've done some refactoring that affects your PR - the clients and models packages are now a single package msgraph, with the models together alphabetically in models.go.

I also had to merge the same golangci-lint fix to the GH action so you'll probably need to revert that file.

If you can take a look at the above this should be good to merge. Thanks again!

clients/conditionalaccesspolicy.go Outdated Show resolved Hide resolved
clients/conditionalaccesspolicy_test.go Outdated Show resolved Hide resolved
Copy link
Owner

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes @liammoat, this LGTM!

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