Skip to content

GitHubRepositories: Add Get/Set/Remove GitHub Repository Team Permissions #300

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Dec 24, 2020
Merged

GitHubRepositories: Add Get/Set/Remove GitHub Repository Team Permissions #300

merged 10 commits into from
Dec 24, 2020

Conversation

X-Guardian
Copy link
Contributor

@X-Guardian X-Guardian commented Nov 7, 2020 •
edited by HowardWolosky
Loading

Description

This PR adds the following functions to the GitHubRepositories module:

  • Get-GitHubRepositoryTeamPermission
  • Set-GitHubRepositoryTeamPermission
  • Remove-GitHubRepositoryTeamPermission

Issues Fixed

Resolves #307

References

Checklist

  • You actually ran the code that you just wrote, especially if you did just "one last quick change".
  • Comment-based help added/updated, including examples.
  • Static analysis is reporting back clean.
  • New/changed code adheres to our coding guidelines.
  • Formatters were created for any new types being added.
  • New/changed code continues to support the pipeline.
  • Changes to the manifest file follow the manifest guidance.
  • Unit tests were added/updated and are all passing. See testing guidelines. This includes making sure that all pipeline input variations have been covered.
  • Relevant usage examples have been added/updated in USAGE.md.
  • If desired, ensure your name is added to our Contributors list

@X-Guardian X-Guardian marked this pull request as draft November 7, 2020 15:35
@X-Guardian X-Guardian marked this pull request as ready for review November 7, 2020 15:41
@HowardWolosky HowardWolosky added api completeness This is basic API functionality that hasn't been implemented yet. api-repositories Work to complete the API's defined here: https://developer.github.com./v3/repos/ labels Nov 8, 2020
@HowardWolosky HowardWolosky self-assigned this Nov 8, 2020
@HowardWolosky
Copy link
Member

Welcome back to the project, @X-Guardian! Apologies in the delay in getting these two PR's reviewed. Things have been a bit busy for me with my primary work. I promise to get to both of these PR's within the next few days though. Thanks again for your contributions!

HowardWolosky
HowardWolosky requested changes Dec 2, 2020
View reviewed changes
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Thanks so much for this contribution, @X-Guardian (and my sincere apologies on the delayed review...I'll blame it on the holidays).

By and large this change looks great. The majority of the requests are just formatting the GitHub noun of "Team" to have a capital 'T' as opposed to a lower-case 't'.

The one design choice that I'm confused by however is the creation of your own return object in lieu of the GitHub object for Get-*. My inclination is to use the object that GitHub is returning, and add to it any additional information we think that the user might need....

@ghost ghost added the needs-author-feedback label Dec 2, 2020
@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky
Copy link
Member

Regarding the Team capitalization...feel free to ignore all of those requests. I just re-read through their documentation, and GitHub doesn't capitalize their nouns, so I don't feel that we need to either.

@HowardWolosky HowardWolosky mentioned this pull request Dec 2, 2020
Merged
10 tasks
@ghost ghost removed the needs-author-feedback label Dec 5, 2020
X-Guardian
X-Guardian commented Dec 5, 2020
View reviewed changes
Copy link
Contributor Author

@X-Guardian X-Guardian left a comment

Choose a reason for hiding this comment

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

@HowardWolosky, ready for the next review.

HowardWolosky
HowardWolosky previously approved these changes Dec 6, 2020
View reviewed changes
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

Update looks good. Thanks for the quick turnaround.
Two small nits that I can apply myself once the review is submitted.
Will kick off a new pipeline build for verification now.

Thanks again!

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky
Copy link
Member

@X-Guardian -- Can you resolve these conflicts locally to avoid the line ending changes that the web editor causes?

@X-Guardian X-Guardian force-pushed the GitHubRepositories-Team-Permissions branch from 048e95c to 0888e0e Compare December 7, 2020 00:08
@X-Guardian
Copy link
Contributor Author

@HowardWolosky, conflicts resolved. Ready for the next review.

@X-Guardian
Copy link
Contributor Author

@HowardWolosky, any update on getting this PR merged?

@HowardWolosky
Copy link
Member

My apologies. Will get to this tomorrow. I had started looking at it earlier, but I think the rebase threw me off, as I was getting confused why tests were being deleted as part of the updates, and needed to devote more time to re-reviewing the whole thing.

@HowardWolosky
Copy link
Member

This is actually queued for review again this afternoon. Thanks for your patience.

HowardWolosky
HowardWolosky requested changes Dec 22, 2020
View reviewed changes
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

So very sorry for the delayed PR on this update. Holiday time has been pretty busy for me over here. Still, I should have communicated a bit better on this. Again, my apologies.

Your contributions, as always, are very much appreciated.

A few updates to this are being requested. Almost all of them have suggested changes that you can simply apply.

The main question that I have here (and I can't recall if I had the same concern the last time) is whether or not we need OrganizationName as an additional parameter to these methods, or if we can just re-use OwnerName as the OrganizationName as well. Do you know if it's possible for an organization's team to be given permission to an individual person's repo ? (I don't think you can).

GitHubRepositories.ps1 Outdated
ValueFromPipelineByPropertyName,
ParameterSetName = 'TeamSlugUri')]
[ValidateNotNullOrEmpty()]
[string] $OrganizationName,
Copy link
Member

Choose a reason for hiding this comment

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

Same question as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ghost ghost removed the needs-author-feedback label Dec 22, 2020
HowardWolosky
HowardWolosky requested changes Dec 22, 2020
View reviewed changes
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

All the updates here look good. Given our conversation around OrganizationName, I'll await a final update that removes OrganizationName and just has it reuse the OwnerName for both OwnerName and OrganizationName. Thanks again.

@HowardWolosky HowardWolosky mentioned this pull request Dec 23, 2020
Closed
@ghost ghost removed the needs-author-feedback label Dec 24, 2020
@X-Guardian
Copy link
Contributor Author

@HowardWolosky, I've removed the OrganizationName parameter, so the PR is ready for the next review.

HowardWolosky
HowardWolosky approved these changes Dec 24, 2020
View reviewed changes
Copy link
Member

@HowardWolosky HowardWolosky left a comment

Choose a reason for hiding this comment

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

This update looks good. Will kick off a pipeline and then merge upon completion.
Thanks!

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky HowardWolosky merged commit 22e3d7b into microsoft:master Dec 24, 2020
@X-Guardian X-Guardian deleted the GitHubRepositories-Team-Permissions branch December 27, 2020 10:21
@X-Guardian
Copy link
Contributor Author

@HowardWolosky, thankyou for merging this. Do you have any plans for releasing a new version?

@HowardWolosky
Copy link
Member

thankyou for merging this. Do you have any plans for releasing a new version?

I'll get the next version released tomorrow or this weekend.

@HowardWolosky
Copy link
Member

I'll get the next version released tomorrow or this weekend.

Had a slight delay, but it's now published. Thanks again for your help here!

@X-Guardian
Copy link
Contributor Author

No problem, thanks Howard.

@HowardWolosky HowardWolosky mentioned this pull request Jul 26, 2022
Closed
@HowardWolosky HowardWolosky mentioned this pull request Feb 27, 2023
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HowardWolosky HowardWolosky HowardWolosky approved these changes

Assignees

@HowardWolosky HowardWolosky

Labels
api completeness This is basic API functionality that hasn't been implemented yet. api-repositories Work to complete the API's defined here: https://developer.github.com./v3/repos/
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Teams to Repo
2 participants
@X-Guardian @HowardWolosky