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

GitHubTeams: Add New/Update/Remove Functions #257

Merged
merged 28 commits into from
Aug 11, 2020

Conversation

X-Guardian
Copy link
Contributor

@X-Guardian X-Guardian commented Jun 29, 2020

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.

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.

Type view formats have been added for the GitHub.TeamSummary and GitHub.Team types.

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

@HowardWolosky HowardWolosky added api completeness This is basic API functionality that hasn't been implemented yet. api-teams Work to complete the API's defined here: https://developer.github.com/v3/teams/ labels Jun 29, 2020
@X-Guardian X-Guardian marked this pull request as ready for review July 6, 2020 17:28
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.

Some additional feedback for you to consider.

PowerShellForGitHub.psd1 Outdated Show resolved Hide resolved
Tests/GitHubTeams.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubTeams.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubTeams.tests.ps1 Outdated Show resolved Hide resolved
GitHubTeams.ps1 Outdated Show resolved Hide resolved
GitHubTeams.ps1 Outdated Show resolved Hide resolved
GitHubTeams.ps1 Show resolved Hide resolved
GitHubTeams.ps1 Show resolved Hide resolved
GitHubTeams.ps1 Show resolved Hide resolved
GitHubTeams.ps1 Outdated Show resolved Hide resolved
@HowardWolosky HowardWolosky added the waiting for update Waiting for an update to the PR before the next review label Jul 7, 2020
@X-Guardian
Copy link
Contributor Author

I've re-added Set-StrictMode -Version 1.0 to the unit tests, as setting it in common.ps1 is out of scope for the tests and not working.

@X-Guardian
Copy link
Contributor Author

@HowardWolosky, this PR is ready for the next review.

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky
Copy link
Member

This had some test failures, likely do to timing issues. I see two options forward:

  1. Easy option: Follow the pattern from Attempting to increase reliability of some Pester tests #265 and judiciously add some sleeps to help with the timing
  2. Harder option: Try out Improve reliability (and speed) of UT's be transitioning to rely on a mock of Invoke-WebRequest #267 for most of your tests.

I recommend the first option for this PR.

@X-Guardian
Copy link
Contributor Author

@HowardWolosky, I've added delays to the failing tests. Can you re-run the CI?

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@X-Guardian
Copy link
Contributor Author

@HowardWolosky, this PR is ready for the next review.

@HowardWolosky
Copy link
Member

@X-Guardian - I have a backlog of work that I'm getting through, and so I haven't had the chance to re-review your updated PR's quite yet. I should get to your PR's by the end of the weekend. I appreciate your patience here.

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@X-Guardian
Copy link
Contributor Author

@HowardWolosky, can you run the CI for this PR?

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@X-Guardian
Copy link
Contributor Author

@HowardWolosky, can you run the CI for this PR?

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@X-Guardian
Copy link
Contributor Author

@HowardWolosky, can you run the CI for this PR?

@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).

GitHubTeams.ps1 Outdated
$repositoryFullName = @()
foreach ($repository in $RepositoryName)
{
$repositoryFullName += "$OrganizationName/$Repository"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$repositoryFullName += "$OrganizationName/$Repository"
$repositoryFullName += "$OrganizationName/$repository"

@@ -1,240 +1,245 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

Can you update the line endings in this file so that we only see the actual diffs of the changes made?

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 because this file was originally saved as UTF-8 with BOM. Editing it within GitHub removes the BOM.

Tests/Common.ps1 Outdated Show resolved Hide resolved
Tests/GitHubTeams.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubTeams.tests.ps1 Outdated Show resolved Hide resolved
GitHubTeams.ps1 Show resolved Hide resolved
GitHubTeams.ps1 Show resolved Hide resolved
GitHubTeams.ps1 Outdated Show resolved Hide resolved
GitHubTeams.ps1 Outdated Show resolved Hide resolved
@HowardWolosky HowardWolosky merged commit 6a51601 into microsoft:master Aug 11, 2020
@HowardWolosky
Copy link
Member

...and it's in! Thanks for your contribution here!

@X-Guardian X-Guardian deleted the GitHubTeams-Add-New-Remove branch August 12, 2020 06:34
HowardWolosky added a commit that referenced this pull request Aug 14, 2020
#257 did a fantastic job closing the gap on missing functionality in the module related to GitHub Teams.
This PR just adds onto that work by providing options that will reduce the need for additional queries during common operations.

* `Get-GitHubTeam`: **Breaking Change** Looking up a team by its `ID` has been deprecated per GitHub documentation.  Removed that functionality and added the ability to look up by `slug` instead.  This also means that you can now pipe in a team and get back the specific result without needing to filter through all org results.

* `Get-GitHubTeamMember`: Updated to use the `slug` instead of the team `ID`.

* `New-GitHubTeam`: Added ability to pass in the `ParentTeamId` (also via the pipeline) to avoid the need to query through all org teams.

* `Set-GitHubTeam`: Added ability to pass in the team `slug` (also via the pipeline) to avoid the need to query through all org teams.  Similar to `New-GitHubTeam`, also added the ability to pass-in the `ParentTeamId` to avoid the full org team lookup.

* `Remove-GitHubTeam`: Added ability to pass in the team `slug` (also via the pipeline) to avoid the need to query through all org teams.

* Added `Rename-GitHubTeam` as a helper/wrapper on top of `Set-GitHubTeam`

* Added additional pipeline tests for existing functions

* Added new tests for `Rename-GitHubTeam` and `Get-GitHubTeamMember`

* Small update to formatters to simplify the display of the team parent

References: [GitHub Teams API](https://developer.github.com/v3/teams/)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api completeness This is basic API functionality that hasn't been implemented yet. api-teams Work to complete the API's defined here: https://developer.github.com/v3/teams/ waiting for update Waiting for an update to the PR before the next review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants