-
Notifications
You must be signed in to change notification settings - Fork 30
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
Support teams, team repositories, and team members #165
Conversation
Thanks @japborst for using Steampipe 👍. The tables and all the documents around it look great 😄 . There are a few points which we would like to address before releasing them:
Please let us know how you feel about the above approach, looking forward to your thoughts. |
github/table_github_team.go
Outdated
//// HYDRATE FUNCTIONS | ||
|
||
func tableGitHubTeamGet(ctx context.Context, d *plugin.QueryData, h *plugin.HydrateData) (interface{}, error) { | ||
var org string |
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.
Here and below, the code to support both cases is pretty ugly. Suggestions are welcome.
I would prefer to not do that. Namely, if we collect to an array (repository names, member logins), that doesn't give us any additional information. I guess one could flatten that array and join with the user table, but I'm not sure what we're really gaining then, as joining is definitely more convenient. |
I took a closer look at the information available in the GitHub API around team memberships + team repository relationships, and it seems like there's some additional information available for each relationship that GitHub does not return from a regular list call that would be useful to include in a separate table representing the relationship. For instance:
In some of the tables that @misraved had mentioned, like the But, notably for organization members, we don't include each member's So it seems like actually having separate tables for relationships can be beneficial, as there's information about the relationship that you need to make individual API calls for, e.g., one API call per organization member to get its state and role information. These types of calls don't seem great to include in the parent table, like We could follow what Terraform does in general for these types of tables, which would mean the two additional tables in this PR would be named correctly. However, @japborst, if we were to proceed in this way, one suggestion I'd have is to reduce the columns down in both. For instance, in the @johnsmyth @japborst Interested in your thoughts on the proposal above (it came out a lot longer than I thought it'd be). Thanks! |
Thanks, @cbruno10! Agree to your points. Especially
|
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.
@japborst I'm also looking into why your second query is failing you mentioned in your original pull request comment, and what's a solution/workaround to allow those queries to work properly. I'll add another comment once I have more findings.
In the meantime, I've added some more suggestions, and if you have any questions please let me know, thanks!
Hey @japborst , I was looking at how we could get your second example working (joining I believe that adding a So now if a user runs
If a user runs The Because users can only list teams they're either a part of or have visibility to (which I believe requires the user to at least be a member of an organization they're attempting to list teams for), having the I've implemented and tested these changes in https://github.com/turbot/steampipe-plugin-github/tree/add-parent-hydrate-team, and they appear to be working well so far (your original query now runs ok as well). In that branch, I've also updated a few other items:
Can you please have a look at my proposal above along with the branch, and let me know if this is aligned with your original intentions for this PR, and if you have any other feedback/suggestions? Thanks! |
…o github_team and github_myteam tables, use parent item in github_team get function
Nice @cbruno10! That works. I've pushed some final changes. Good to go from my end ✅ |
Thanks so much @japborst for all of the hard work and great discussions! |
My first
go
and first steampipe contribution, so bear with me 😄Changes
Added 3 new tables:
github_team
github_team_members
github_team_repositories
Considerations
I feel there's quite some duplicate code already present in the repo on how the list results are handled, but I feel that's outside the scope of this PR to fix.
Open questions
For some reason, the following works
but this doesn't, even though it should work and I don't see clear difference between
github_team_repository
andgithub_team_member
.