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

Add github_teams data source #725

Merged
merged 9 commits into from
Apr 9, 2021
Merged

Add github_teams data source #725

merged 9 commits into from
Apr 9, 2021

Conversation

reinoudk
Copy link
Contributor

The github_teams data source is useful to get all teams of an organization without having prior knowledge of their names/slug.
One of the use cases is to synchronize these teams and their members to some external system.

This uses the GraphQL API and returns all teams of an organization including their members in a single request per 100 teams.

@majormoses
Copy link
Contributor

When I read the data source name I make a leap that I can ask for specific teams when this only looks to solve the "all" use case. I think that should be in the name of the data source if that is the scope of this source.

There is a data source for a single github_team, it does not provide a list of child teams or a way to traverse it. With a data source named as such I would expect that I can optionally provide a specific parent to start from and walk all the child objects.

@reinoudk
Copy link
Contributor Author

@majormoses Good point. Maybe it would be good to discuss this in #726 ?

This better reflects that the data source fetches all teams for an organization.
Setting the attribute to true causes the query to only return teams at the root of the organization.
@reinoudk
Copy link
Contributor Author

I've changed the name of the data source to github_organization_teams, based on the discussion in #726. I'd like to add a github_team_child_teams data source as well, but will create a different PR for that.

@jcudit
Copy link
Contributor

jcudit commented Apr 1, 2021

Seems on the right track from what I'm reading over in the related issue and from the code itself. Can we also get an update to website/github.erb to add a link out to the new docs that are added?

@reinoudk
Copy link
Contributor Author

reinoudk commented Apr 1, 2021

Ah, somehow missed that. Fixed.

Copy link
Contributor

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

LGTM

@jcudit jcudit added this to the v4.7.0 milestone Apr 6, 2021
variables["cursor"] = githubv4.NewString(query.Organization.Teams.PageInfo.EndCursor)
}

d.SetId(strconv.FormatInt(time.Now().Unix(), 10))
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting approach here. 🤔 I'm unsure if this will cause unwanted behaviour and have not seen this pattern used before. 👍🏾 on trying it but we'll need to keep an eye out for fixes that may need to occur due to choosing what seems to be a non-deterministic ID.

I see a similar data source pins to the overall org ID:

// github/data_source_github_organization.go
d.SetId(strconv.FormatInt(organization.GetID(), 10))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is something I picked up from Hashicorp's tutorials (https://learn.hashicorp.com/tutorials/terraform/provider-setup#implement-read) and used more often but I think that it causes -refresh=false to have no effect. I've added a commit to change this.

(Btw, I was a bit surprised that the ID returned by the GraphQL API is already a string, it seems to be something different from the v3 REST API.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, added another change. In the previous commit I added the value of root_teams_only to the ID so it would be unique between multiple instances of the data source (edge case). But that is of course not necessary, so I simplified that again.

Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

Added a comment about the ID chosen, but overall looks like a quality contribution! Will pause for a couple of days in case there is more feedback but plan on shipping this as-is with the next release.

reinoudk added 2 commits April 7, 2021 14:05
This makes the ID deterministic and prevents overriding the  behaviour.
Adding the root_teams_only property to the ID is not needed.
Two datasources having the same ID property is no problem.
@jcudit jcudit merged commit c2cead9 into integrations:master Apr 9, 2021
@reinoudk reinoudk deleted the data-source-teams branch April 9, 2021 11:54
kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
* Add github_teams data source

* Add id field to teams field of github_teams data source

* Add website docs for github_teams data source

* Fix linting errors in data_source_github_teams.go

* Rename github_teams to github_organization_teams

This better reflects that the data source fetches all teams for an organization.

* Add root_teams_only attribute to github_organization_teams

Setting the attribute to true causes the query to only return teams at the root of the organization.

* Add link to github_organization_teams docs in sidebar

* Use organization ID in datasource ID

This makes the ID deterministic and prevents overriding the  behaviour.

* Simplify ID of github_organization_teams data source

Adding the root_teams_only property to the ID is not needed.
Two datasources having the same ID property is no problem.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants