Skip to content
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

Merged
merged 9 commits into from
Apr 12, 2018

Conversation

paddie
Copy link
Contributor

@paddie paddie commented Feb 19, 2018

Initial work on the data source that would fix #64

@paddie
Copy link
Contributor Author

paddie commented Feb 19, 2018

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.

@paddie paddie force-pushed the pagerduty-team-data-source branch from 5a4f27f to c2780e1 Compare February 20, 2018 09:43
@rlhh
Copy link

rlhh commented Mar 12, 2018

👍 Would love to see this merged as it nicely complements the existing pagerduty_team resource

@heimweh
Copy link
Collaborator

heimweh commented Mar 19, 2018

Hi @paddie,

thank you so much for this contribution! 👍

This looks good to me at a first glance, left one comment regarding Description.

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! 🙂

@paddie
Copy link
Contributor Author

paddie commented Mar 21, 2018

@heimweh on it. I can't actually see your comment?

},
"description": {
Type: schema.TypeString,
Required: false,
Copy link
Collaborator

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.

@paddie
Copy link
Contributor Author

paddie commented Mar 21, 2018

@heimweh Alright, I also added a Description alongside the required field and added a readme entry

@paddie
Copy link
Contributor Author

paddie commented Apr 5, 2018

@heimweh what's the status on this one?

Copy link
Collaborator

@heimweh heimweh left a 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:

  1. Could we rename the files data_pagerduty_team*.go to data_source_pagerduty_team*.go just to keep things consistent.

  2. 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.

  3. 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"),
Copy link
Collaborator

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}"
Copy link
Collaborator

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}"

@paddie
Copy link
Contributor Author

paddie commented Apr 11, 2018

@heimweh nice catch. Made the mentioned changes, maybe double check the pagerduty.erb changes for me.

Copy link
Collaborator

@heimweh heimweh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @paddie,

thank you so much for your work on this.

Tests are passing and this LGTM 👍

@heimweh heimweh merged commit 1b30e5e into PagerDuty:master Apr 12, 2018
@paddie
Copy link
Contributor Author

paddie commented Apr 12, 2018

Awesome, thanks for the excellent code review @heimweh !

@paddie paddie deleted the pagerduty-team-data-source branch April 12, 2018 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] pagerduty_team data source
3 participants