-
Notifications
You must be signed in to change notification settings - Fork 59
feat: Add boundary_role
data source
#743
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
Conversation
a59ad2a
to
7fe6648
Compare
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 for the contribution! Have a couple of comments
Steps: []resource.TestStep{ | ||
{ | ||
Config: testConfig(url, roleReadGlobal), | ||
Check: resource.ComposeTestCheckFunc( |
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.
What are your thoughts on adding additional checks for some of the other fields in the data source, like grant_strings
or principal_ids
?
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.
Sorry, I should have been more explicit in the earlier comment. Thanks for adding the checks to those fields. Could we include all of the fields that we expect to be returned? I think the only remaining one now is grant_scope_ids
?
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.
I should have added those checks in the initial commit, my mistake. Thanks for spotting that !
Thanks for the contribution! In addition to what Mike mentioned above could you also rebase against |
34d1de6
to
0c0d731
Compare
Thank you for the review and the time you spent on it 🙏 Changes made following your feedbacks :
|
Steps: []resource.TestStep{ | ||
{ | ||
Config: testConfig(url, roleReadGlobal), | ||
Check: resource.ComposeTestCheckFunc( |
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.
Sorry, I should have been more explicit in the earlier comment. Thanks for adding the checks to those fields. Could we include all of the fields that we expect to be returned? I think the only remaining one now is grant_scope_ids
?
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 for the contribution <3
This PR adds a
boundary_role
data sourceAcceptance tests
$ TF_ACC=1 go test -run TestAccRoleRead -v ./internal/provider === RUN TestAccRoleReadGlobal --- PASS: TestAccRoleReadGlobal (6.41s) === RUN TestAccRoleReadOrg --- PASS: TestAccRoleReadOrg (7.74s) PASS ok github.com/hashicorp/terraform-provider-boundary/internal/provider 15.070s
Manual tests
Ouputs:
closes #548