-
Notifications
You must be signed in to change notification settings - Fork 62
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 RoleEligibilityScheduleRequestClient #204
Conversation
Hi @JonasBak, thanks for submitting this, your contribution is very appreciated! Apologies for the delayed response - this is looking good so far! When it comes to the tests, don't worry about the permissions as we'll add these as needed to our testing principal (it doesn't have the privilege to create its own role assignments anyway). |
I lost access to the "sandbox tenant" I used when I started working on this, so I didn't prioritize it for a while, but it looks like someone created their own PR 🙌 If that gets merged, I'll just update my PR to the terraform provider. |
I rebased and added a basic test |
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.
@JonasBak Thanks for continuing to work on this and for adding the test. This is off to a great start and with some extra work to support all the fields and methods for this resource, this will be a great addition. I've made some comments below - if you can take a look at these, I'll be happy to re-review and we can look to try and get this merged. Thanks!
msgraph/models.go
Outdated
type UnifiedRoleEligibilityScheduleRequest struct { | ||
DirectoryObject | ||
|
||
Action *string `json:"action,omitempty"` | ||
DirectoryScopeId *string `json:"directoryScopeId,omitempty"` | ||
Justification *string `json:"justification,omitempty"` | ||
PrincipalId *string `json:"principalId,omitempty"` | ||
RoleDefinitionId *string `json:"roleDefinitionId,omitempty"` | ||
ScheduleInfo *interface{} `json:"scheduleInfo,omitempty"` | ||
} |
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.
This type of object isn't a directoryObject
, so it should not embed the DirectoryObject
struct and should instead have an Id
field, e.g.
Id *string `json:"id,omitempty"`
It looks like the model is missing quite a lot of fields? From the documentation: approvalId
, appScopeId
, completedDateTime
, createdDateTime
, directoryScopeId
, isValidationOnly
, status
, targetScheduleId
, and ticketInfo
.
Additionally, the ScheduleInfo
field should have a RequestSchedule
struct type defined for it. The Action
field should also be a custom aliased type so that we can define constants for the values, e.g.
Action UnifiedRoleScheduleRequestAction `json:"action,omitempty"`
ScheduleInfo *RequestSchedule `json:"scheduleInfo,omitempty"`
// this goes in valuetypes.go
type UnifiedRoleScheduleRequestAction = string
const (
UnifiedRoleScheduleRequestActionAdminAssign UnifiedRoleScheduleRequestAction = "adminAssign"
UnifiedRoleScheduleRequestActionAdminExtend UnifiedRoleScheduleRequestAction = "adminExtend"
UnifiedRoleScheduleRequestActionAdminRemove UnifiedRoleScheduleRequestAction = "adminRemove"
UnifiedRoleScheduleRequestActionAdminRenew UnifiedRoleScheduleRequestAction = "adminRenew"
UnifiedRoleScheduleRequestActionAdminUpdate UnifiedRoleScheduleRequestAction = "adminUpdate"
UnifiedRoleScheduleRequestActionSelfActivate UnifiedRoleScheduleRequestAction = "selfActivate"
UnifiedRoleScheduleRequestActionSelfDeactivate UnifiedRoleScheduleRequestAction = "selfDeactivate"
UnifiedRoleScheduleRequestActionSelfExtend UnifiedRoleScheduleRequestAction = "selfExtend"
UnifiedRoleScheduleRequestActionSelfRenew UnifiedRoleScheduleRequestAction = "selfRenew"
UnifiedRoleScheduleRequestActionUnknownFutureValue UnifiedRoleScheduleRequestAction = "unknownFutureValue"
)
Lastly, although this is more of a niggle, could we move this struct furthe rup the file to around line 1691 to maintain ordering as much as possible (I realise there are some other structs appended to the end of this file, they'll get tidied up separately).
var actionAdminAssign string = "adminAssign" | ||
var actionAdminRemove string = "adminRemove" |
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.
As per above, these values should be constants in valuetypes.go
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.
We will also want to support the List
and Cancel
operations for this resource
}, | ||
}) | ||
if err != nil { | ||
return status, fmt.Errorf("RoleAssignments.BaseClient.Get(): %v", err) |
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.
We'll want to check through all the error messages to make sure they reflect the correct client and method.
Thanks for the feedback 🙌 I have some problems with the test being flaky, as it sometimes fails with |
Also, should I add the |
@JonasBak I'd suggest leaving out the I agree it's a little odd to have an effective deletion (i.e. removal/revocation) via the same method and operation as Create. However, for better or worse, this is the way the API is designed and for simplicity it's maybe better to expose this implementation detail to the user and leave it to them to decide how that fits in with their use case. For example with the AzureAD TF provider, we'd still have explicit Create, Update and Destroy operations even if they all call the same 'Create' method but with different arguments. When it comes to replication delays, the SDK has some built in retry capability but needs to be configured using a Hope this helps! |
Seems like I just didn't understand how the different "role types" work when writing the test 😅 But now it passes ✅ I haven't added a test for the Cancel function, as that requires me to first activate it, which seems to use the |
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.
Thanks again for your work on this @JonasBak. This is looking good to me, I think it's fine to omit the test for the Cancel
method for now, we can look at it again once we can support the entire workflow.
The test failure seems unrelated, so I'm happy to merge this. Thanks again!
This PR adds a
RoleEligibilityScheduleRequestClient
, which lets us use the PIM API for managing role eligibilities. This is part of a PR to the https://github.com/hashicorp/terraform-provider-azuread repo, where we would like to be able to manage role eligibilities through terraform.I havn't written any tests, as I wanted to get some input on how this should be done. The user/service principal requires a role and a permission to be able to use the API. Should I set up the permissions as part of the test (and remove them after)? Should I assume the calling service principal has the correct roles and permissions?