-
Notifications
You must be signed in to change notification settings - Fork 280
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
feat: Add azuredevops_group_entitlment #870
Conversation
This change is based on PR microsoft#707, most of the work should be credited to marley-ma. microsoft#707 Co-authored-by: marley-ma <https://github.com/marley-ma> Co-authored-by: stanleyz <https://github.com/stanleyz>"
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.
@stanleyz A few changes requested, others looks good to me.
Document website/docs/r/group_entitlement.html.markdown
need to be documented in azuredevops.erb
azuredevops/internal/service/memberentitlementmanagement/resource_group_entitlement.go
Outdated
Show resolved
Hide resolved
azuredevops/internal/service/memberentitlementmanagement/resource_group_entitlement.go
Outdated
Show resolved
Hide resolved
azuredevops/internal/service/memberentitlementmanagement/resource_group_entitlement.go
Outdated
Show resolved
Hide resolved
azuredevops/internal/service/memberentitlementmanagement/resource_group_entitlement.go
Outdated
Show resolved
Hide resolved
azuredevops/internal/service/memberentitlementmanagement/resource_group_entitlement.go
Outdated
Show resolved
Hide resolved
if !uuidRegexp.MatchString(upn) { | ||
return nil, fmt.Errorf("Only UUID values can used for import [%s]", upn) | ||
} |
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 !uuidRegexp.MatchString(upn) { | |
return nil, fmt.Errorf("Only UUID values can used for import [%s]", upn) | |
} | |
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 is still required, uuid.parse doesn't valid the same as this regex. See the function TestGroupEntitlement_Import_TestInvalidValue
in resource_group_entitlement_test.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.
The d.id()
is from service which we can trust is a valid UUID. If the ID is not a valid UUID, there is a something wrong with the service
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.
normally yes, however this function is for importing existing resource, you can't guarantee the users will always have a valid UUID supplied, it's not a big deal but additional check with nice error message always helps.
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.
UUID already check by _, err := uuid.Parse(d.Id()), no need to verify twice
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.
cool, updated. That was my impression in the first look, double checked, there was a logic error in the test, not the uuid.Parse
. I've updated the test and import function.
…rce_group_entitlement.go Co-authored-by: xuzhang3 <57888764+xuzhang3@users.noreply.github.com>
Co-authored-by: xuzhang3 <57888764+xuzhang3@users.noreply.github.com>
|
This change is based on PR #707, most of the work should be credited to marley-ma.
#707
"
All Submissions:
What about the current behavior has changed?
Issue Number:
#700
Issue Number: 700
Does this introduce a change to
go.mod
,go.sum
orvendor/
?Does this introduce a breaking change?
Any relevant logs, error output, etc?
(If it’s long, please paste to https://ghostbin.com/ and insert the link here.)
Other information