-
Notifications
You must be signed in to change notification settings - Fork 593
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
ACCT-4459: Support for domain scoped roles #1095
Conversation
…ups into their own files
changelog detected ✅ |
Codecov Report
@@ Coverage Diff @@
## master #1095 +/- ##
==========================================
+ Coverage 49.94% 50.29% +0.34%
==========================================
Files 115 119 +4
Lines 10991 11455 +464
==========================================
+ Hits 5490 5761 +271
- Misses 4338 4463 +125
- Partials 1163 1231 +68
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
account_members.go
Outdated
// upon member invitation. We recommend upgrading to CreateAccountMemberWithPolicies to use policies. | ||
// | ||
// API reference: https://api.cloudflare.com/#account-members-add-member | ||
func (api *API) CreateAccountMember(ctx context.Context, accountID string, emailAddress string, roles []string) (AccountMember, error) { |
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.
if we're overhauling the invitations, let's update CreateAccountMember
method signature to match the experimental client. this will also allow you to cut down on some of the boilerplate code we're repeating and putting into CreateAccountMemberInternal
.
(incomplete but hopefully enough to get you started example)
type CreateAccountMemberParams struct {
Email string `json:"email"`
Roles []string `json:"roles,omitempty"`
Policies []Policy `json:"policies,omitempty"`
Status string `json:"status,omitempty"`
}
func (api *API) CreateAccountMember(ctx context.Context, rc *ResourceContainer, params CreateAccountMemberParams) (AccountMember, error) {
invite := AccountMemberInvitation{
Email: params.Email,
}
if len(params.Roles) > 0 {
invite.Roles = params.Roles
}
// ... etc
}
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.
Good call, thanks! Can you confirm that this is the right way to go about things?
- Re-write
CreateAccountMember
to match the experimental client - Remove the
CreateAccountMemberInternal
method - Re-factor utility methods (
CreateAccountMemberWithStatus
,CreateAccountMemberWithRoles
, etc) to just callCreateAccountMember
- the alternative I supposed is just removing the utility methods?
Here's my current CreateAccountMember
impl, let me know if it looks okay and the above is correct and I'll push :)
func (api *API) CreateAccountMember(ctx context.Context, rc *ResourceContainer, params CreateAccountMemberParams) (AccountMember, error) {
if rc.Level != AccountRouteLevel {
return AccountMember{}, fmt.Errorf(errInvalidResourceContainerAccess, rc.Level)
}
invite := AccountMemberInvitation{
Email: params.EmailAddress,
Status: params.Status,
}
roles := []AccountRole{}
for i := 0; i < len(params.Roles); i++ {
roles = append(roles, AccountRole{ID: invite.Roles[i]})
}
err := validateRolesAndPolicies(roles, params.Policies)
if err != nil {
return AccountMember{}, err
}
if params.Roles != nil {
invite.Roles = params.Roles
} else if params.Policies != nil {
invite.Policies = params.Policies
}
uri := fmt.Sprintf("/accounts/%s/members", rc.Identifier)
res, err := api.makeRequestContext(ctx, http.MethodPost, uri, invite)
if err != nil {
return AccountMember{}, err
}
var accountMemberListResponse AccountMemberDetailResponse
err = json.Unmarshal(res, &accountMemberListResponse)
if err != nil {
return AccountMember{}, fmt.Errorf("%s: %w", errUnmarshalError, err)
}
return accountMemberListResponse.Result, nil
}
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.
looks spot on! nice one.
abc8db2
to
c4f8f2e
Compare
account_members_test.go
Outdated
@@ -215,8 +252,12 @@ func TestCreateAccountMemberWithStatus(t *testing.T) { | |||
} | |||
|
|||
mux.HandleFunc("/accounts/01a7362d577a6c3019a474fd6f485823/members", handler) | |||
accountResource := &ResourceContainer{ |
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 the code docs - https://github.com/cloudflare/cloudflare-go/blob/master/resource.go#L12-L14
// ResourceContainer defines an API resource you wish to target. Should not be
// used directly, use `UserIdentifier`, `ZoneIdentifier` and `AccountIdentifier`
// instead.
while you can do this, i'd recommend using the helpers instead incase we expand this struct. it's also a bit nicer to read 😄 AccountIdentifier("01a7362d577a6c3019a474fd6f485823")
This functionality has been released in v0.53.0. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
As we begin to GA domain-scoped roles, we'd like to add support for these new controls to the Cloudflare Go SDK.
Description
The primary changes here are:
Has your change been tested?
Yes, we have extended existing tests, added new tests, and written a verification script
Screenshots (if appropriate):
n/a
Types of changes
What sort of change does your code introduce/modify?
Checklist: