Skip to content

GitHubTeams: Add New/Update/Remove Functions - #222

Closed
Simon Heather (X-Guardian) wants to merge 19 commits into
microsoft:masterfrom
X-Guardian:GitHubTeams-Add-New-Remove
Closed

GitHubTeams: Add New/Update/Remove Functions#222
Simon Heather (X-Guardian) wants to merge 19 commits into
microsoft:masterfrom
X-Guardian:GitHubTeams-Add-New-Remove

Conversation

@X-Guardian

@X-GuardianSimon Heather (X-Guardian) commented Jun 6, 2020•
edited by HowardWolosky
Loading

Copy link
Copy Markdown
Contributor

This PR has been superseded by PR #257

Description

This PR adds the following functions to the GitHubTeams module:

  • Add-GitHubTeam
  • Update-GitHubTeam
  • Remove-GitHubTeam

It also adds the TeamName parameter to the Get-GitHubTeam function to allow getting team details by team name, which is used by the new functions.

100% coverage Pester tests have also been added for the following functions:

  • Get-GitHubTeam
  • Add-GitHubTeam
  • Update-GitHubTeam
  • Remove-GitHubTeam

Positional Binding has been set as false for the three functions, and Position attributes added to the function's mandatory parameters.

Issues Fixed

None

References

GitHub Teams API

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.
  • Changes to the manifest file follow the manifest guidance.
  • Unit tests were added/updated and are all passing. See testing guidelines.
  • Relevant usage examples have been added/updated in USAGE.md.
  • If desired, ensure your name is added to our Contributors list

@X-Guardian

Simon Heather (X-Guardian) commented Jun 6, 2020

Copy link
Copy Markdown
ContributorAuthor

Can we trigger the CI for this PR?

@HowardWolosky

Howard Wolosky (HowardWolosky) commented Jun 6, 2020

Copy link
Copy Markdown
Contributor

/azp run PowerShellForGitHub-CI

@azure-pipelines

azure-pipelinesBot commented Jun 6, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@X-Guardian
Simon Heather (X-Guardian) marked this pull request as ready for review June 7, 2020 11:11
@X-Guardian

Simon Heather (X-Guardian) commented Jun 7, 2020

Copy link
Copy Markdown
ContributorAuthor

Can we trigger the CI for this PR?

@X-Guardian

Simon Heather (X-Guardian) commented Jun 7, 2020

Copy link
Copy Markdown
ContributorAuthor

I've added the -Force switch to the Remove-GitHubTeams function as detailed in issue #218.

@HowardWolosky

Howard Wolosky (HowardWolosky) commented Jun 8, 2020

Copy link
Copy Markdown
Contributor

/azp run PowerShellForGitHub-CI

@azure-pipelines

azure-pipelinesBot commented Jun 8, 2020

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@X-Guardian

Simon Heather (X-Guardian) commented Jun 8, 2020

Copy link
Copy Markdown
ContributorAuthor

Howard Wolosky (@HowardWolosky) , this PR is ready for review.

@HowardWolosky

Howard Wolosky (HowardWolosky) commented Jun 9, 2020

Copy link
Copy Markdown
Contributor

Thanks Simon Heather (@X-Guardian) ... holding off on this one for another few days until the big pipelining change is in so as not to accumulate further debt. I'll start a review on this soon, but there will be just a little bit of additional work needed to align the new functions to the way pipelining will be working with the rest of the module before it gets merged in.

As an aside, I'm in agreement on removing implicit positional parameters on these and future functions per our discussion around #219.

@HowardWoloskyHoward Wolosky (HowardWolosky) added api-teams Work to complete the API's defined here: https://developer.github.com/v3/teams/ enhancement An issue or pull request introducing new functionality to the project. api completeness This is basic API functionality that hasn't been implemented yet. and removed enhancement An issue or pull request introducing new functionality to the project. labels Jun 18, 2020
HowardWolosky
Howard Wolosky (HowardWolosky) suggested changes Jun 19, 2020
View reviewed changes

@HowardWoloskyHoward Wolosky (HowardWolosky) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome. Thanks for adding in this functionality! Much appreciated.

I have some feedback for you to consider before getting this merged in though. Primarily, it's a matter of adding-in pipeline support to these methods, but there's other feedback as well.

Thanks again.

Comment threadPowerShellForGitHub.psd1 Outdated
Comment threadTests/GitHubTeams.tests.ps1 Outdated
Comment threadGitHubTeams.ps1 Outdated
Comment threadGitHubTeams.ps1
Comment threadGitHubTeams.ps1 Outdated
Comment threadGitHubTeams.ps1
Comment threadTests/GitHubTeams.tests.ps1 Outdated
Comment threadGitHubTeams.ps1 Outdated
Comment threadTests/GitHubTeams.tests.ps1 Outdated
Comment threadTests/GitHubTeams.tests.ps1 Outdated
HowardWolosky
Howard Wolosky (HowardWolosky) suggested changes Jun 20, 2020
View reviewed changes

@HowardWoloskyHoward Wolosky (HowardWolosky) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great update.

I think that there's still room for improvement in the parameter design to enable better pipeline support and reduce the need for the additional Get-GitHubTeam calls in the normal cases.

Comment threadGitHubTeams.ps1
Comment threadGitHubTeams.ps1
Comment threadGitHubTeams.ps1
Comment threadGitHubTeams.ps1
@HowardWoloskyHoward Wolosky (HowardWolosky) added the waiting for update Waiting for an update to the PR before the next review label Jun 20, 2020
HowardWolosky
Howard Wolosky (HowardWolosky) suggested changes Jun 27, 2020
View reviewed changes

@HowardWoloskyHoward Wolosky (HowardWolosky) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some additional feedback after looking through this again.

Comment threadGitHubTeams.ps1 Outdated
Comment threadGitHubTeams.ps1
Comment threadUSAGE.md Outdated
Comment threadGitHubTeams.ps1
Comment threadGitHubTeams.ps1 Outdated
Comment threadGitHubTeams.ps1 Outdated
Comment threadGitHubTeams.ps1
Comment threadGitHubTeams.ps1
Comment threadGitHubTeams.ps1
@X-GuardianSimon Heather (X-Guardian) mentioned this pull request Jun 28, 2020
Closed
9 tasks
@X-Guardian

Simon Heather (X-Guardian) commented Jun 29, 2020

Copy link
Copy Markdown
ContributorAuthor

Howard Wolosky (@HowardWolosky), do you want me to close this PR and open another one?

@HowardWolosky

Howard Wolosky (HowardWolosky) commented Jun 29, 2020

Copy link
Copy Markdown
Contributor

Yes, please do.

@X-Guardian

Simon Heather (X-Guardian) commented Jun 29, 2020

Copy link
Copy Markdown
ContributorAuthor

This PR has been superseded by PR #257

@HowardWoloskyHoward Wolosky (HowardWolosky) added superseded The PR was replaced by a newer PR and removed waiting for update Waiting for an update to the PR before the next review labels Jun 29, 2020
@HowardWoloskyHoward Wolosky (HowardWolosky) mentioned this pull request Jul 7, 2020
Merged
8 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@HowardWoloskyHoward Wolosky (HowardWolosky)HowardWolosky requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

api completenessThis is basic API functionality that hasn't been implemented yet.api-teamsWork to complete the API's defined here: https://developer.github.com/v3/teams/supersededThe PR was replaced by a newer PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@X-Guardian@HowardWolosky