-
Notifications
You must be signed in to change notification settings - Fork 216
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
Pagerduty team data source #65
Conversation
It seems the tests run but I found writing them a bit confusing, so let me know if there's anything missing or if they could be done better. |
5a4f27f
to
c2780e1
Compare
👍 Would love to see this merged as it nicely complements the existing pagerduty_team resource |
Hi @paddie, thank you so much for this contribution! 👍 This looks good to me at a first glance, left one comment regarding Would you mind adding documentation for this resource as well? You’ll find a basic example here which is for the user data source: https://github.com/terraform-providers/terraform-provider-pagerduty/blob/master/website/docs/d/user.html.markdown Again thank you so much for this! 🙂 |
@heimweh on it. I can't actually see your comment? |
pagerduty/data_pagerduty_team.go
Outdated
}, | ||
"description": { | ||
Type: schema.TypeString, | ||
Required: false, |
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 guess this should be Computed: true
instead of Required
.
@heimweh Alright, I also added a |
@heimweh what's the status on this one? |
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.
Hi @paddie, sorry for the delay regarding this PR.
I had a look and overall this looks super good 👍
I noticed just a few things, part from the inline comments I made:
-
Could we rename the files
data_pagerduty_team*.go
todata_source_pagerduty_team*.go
just to keep things consistent. -
Could we add the data source to the list of data sources in here: https://github.com/terraform-providers/terraform-provider-pagerduty/blob/master/pagerduty/provider.go#L29.
-
We should probably add the new data source here as well: https://github.com/terraform-providers/terraform-provider-pagerduty/blob/master/website/pagerduty.erb#L28
Thank you so much for the work here! it'll be a super nice addition to the provider.
{ | ||
Config: testAccDataSourcePagerDutyTeamConfig(name, description), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccDataSourcePagerDutyUser("pagerduty_team.test", "data.pagerduty_team.by_name"), |
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 think this should be testAccDataSourcePagerDutyTeam
instead of testAccDataSourcePagerDutyUser
.
} | ||
|
||
data "pagerduty_team" "by_name" { | ||
name = "${pagerduty_user.test.name}" |
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 think this should be:
name = "${pagerduty_team.test.name}"
@heimweh nice catch. Made the mentioned changes, maybe double check 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.
Awesome, thanks for the excellent code review @heimweh ! |
Initial work on the data source that would fix #64