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

Adding complete Pipeline support to the entire module #242

Merged
merged 80 commits into from
Jun 18, 2020

Conversation

HowardWolosky
Copy link
Member

@HowardWolosky HowardWolosky commented Jun 17, 2020

Description

Comprehensively adds pipeline support throughout the entire module

This change required examining every method, and in some cases a few breaking changes (noted below) had to be introduced in order to make support work consistently end-to-end. UT's were added for every function, which resuled in a few bugs that were identified and fixed (also noted below).

Breaking changes

Unavoidable Breaks

  • Get-GitHubRepositoryBranch: Name parameter is now BranchName.

A GitHub.Repository object has a name property which is the name of the repo. Piping in the repo object to this method would confuse it, making it search for a branch with the name of the repo, as opposed to the desired effect of just listing out all branches in the repo.

  • *-GitHub*Label: Name parameter is now Label.

Similar to above.
Name was too generic and was causing unintended conflicts when passing in certain objects over the pipeline (like a GitHub.Repository) which also has a name property, making it impossible to pipe in a repository to list all labels (since it was trying to query for a label in that repository named the same as the repo.

  • *-GitHubLabel: Milestone parameter is now MilestoneNumber.

A GitHub.Issue contains a milestone object property, and PowerShell was complaining that there was no way to convert that to an [int64] when an Issue was passed in via the pipeline. The only way to avoid this was to use MilestoneNumber which is the name of the property we add to GitHub.Milestone objects, and it's what we alias to in all other methods that interact with milestones.

  • *-GitHubIssue: Milestone parameter is now MilestoneNumber.

A GitHub.Issue contains a milestone object property, and PowerShell was complaining that there was no way to convert that to an [int64] when an Issue was passed in via the pipeline. The only way to avoid this was to use MilestoneNumber which is the name of the property we add to GitHub.Milestone objects, and it's what we alias to in all other methods that interact with milestones.

  • Get-GitHubLicense: Name parameter is now Key.

The key is what you can query against, but a License object has both a name and a key property, which caused piped object queries to fail with the existing parameter name.

  • Get-GitHubCodeOfConduct: Name parameter is now Key.

Similar to above.
The key is what you can query against, but a Code of Conduct object has both a name and a key property, which caused piped object queries to fail with the existing parameter name.

  • New-ProjectCard: Signature has changed. Instead of specifying ContentId and ContentType, you now just directly specify either IssueId or PullRequestId.

Pipeline input would not have worked without this change. Additionally, this simply provides a cleaner interface for users in general, requiring one less input.

  • Get-GitHubUserContextualInformation: Signature has changed. Instead of specifying SubjectId and SubjectType, you now just directly specify either OrganizationId, RepositoryId, IssueId or PullRequestId.

Similar to above.
Pipeline input would not have worked without this change. Additionally, this simply provides a cleaner interface for users in general, requiring one less input.

Breaks With Workarounds

  • A number of changes to the Comments functions

    • GitHubComments.ps1 was renamed to GitHubIssueComments.ps1 given that there are 5 different types of comments, these functions focus on Issue comments, and there doesn't currently appear to be a path forward for creating a single unified API set for all comment types.

    • All of these methods were renamed from *-GitHubComment to *-GitHubIssueComment, however they were also aliased back to their previous names (for now). You should really migrate to these new names however.

    • The main parameter was renamed from CommentId to Comment, however it was aliased back to CommentId.

  • *-GitHubProject: ArchivedState parameter is now State (but aliased back to ArchivedState to help with migration).

  • *-GitHubProject: Name parameter is now ColumnName (but aliased back to Name)

  • *-GitHubProjectColumn: Name parameter is now ColumnName (but aliased back to Name)

  • Move-GitHubProjectCard: ColumnId parameter is now Column (but aliased back to ColumnId to support pipeline input).

  • Get-GitHubRelease: ReleaseId parameter is now Release (but aliased back to ReleaseId to support pipeline input).

  • Rename-GitHubRepository has been effectively deprectaed. It was built on top of the same endpoint that Set-GitHubRepository uses, but was only modifying a single property (the name). For now, the function remains, but it now just internally calls into Set-GitHubRepository to perform the same action.

  • Set-GitHubRepositoryTopic: Name parameter is now Topic (but aliased back to Name).

  • Get-GitHubUser: User parameter is now UserName (but aliased back to User and Name).

  • Get-GitHubUserContextualInformation: User parameter is now UserName (but aliased back to User and Name).

Bug Fixes:

  • Events had been using a typo'd version of the sailor-v AcceptHeader. This was fixed as part of the migration to using the AcceptHeader constants.
  • Lock-GitHubIssue was not correctly providing the lock_reason value to the API, so that metadata was getting lost. This was caught by new UT's and then fixed.
  • Update-GitHubLabel was modified to accept colors both with and without a leading # sign, just like New-GitHubLabel already supported.

New Features:

  • Get-GitHubGitIgnore has a new RawContent switch which allows you to directly get back the content of the actual .gitignore file.
  • Set-GitHubRepository has been updated to allow users to change the name (rename) the repository at the same time as making any of their other changes. If a new name has been specified, users will be required to confirm the change, or to specify -Confirm:$false and/or -Force to avoid the confirmation.
  • Get-GitHubRepositoryCollaborator has gained a new parameter Affiliation allowing you to filter which collaborators you're interested in querying for.

Minor updates:

  • Fixed the casing of the "microsoft" OwnerName in any examples
  • Fixed the casing of GitHub in the Assignee method names (was Github intead of GitHub
  • Added new configuration property (DisablePipelineSupport) to assist with pipeline development
  • All AcceptHeader usage has migrated to using script-wide constants
  • Split-GitHubUri can now return both components in a single function call if no switch is specified.
  • Added Join-GitHubUri which can reverse what Split-GitHubUri does, based on the configured ApiHostName.
  • Adds type information (via .OUTPUTS and [OutputType()] for every function.
  • Documentation updates reflecting the new pipeline support.

Test changes:

  • Set-StrictMode -Version 1.0 has been specified for all tests to catch access to undeclared variables (which should help catch common typo errors)
  • The workarounds for PSScriptAnalyzer's false complaint for rule PSUseDeclaredVarsMoreThanAssignments have been removed, and all test files now suppress that rule.
  • A test file now exists for every module file, and some tests were moved around to the appropriate file (away from GitHubAnalytics.tests.ps1 which had previously been a dumping ground for tests).
  • A substantial number of new tests have been added. Every function should now be tested with limited exceptions (like for Organizations and Teams where we don't yet have the support in the module to get the account in the appropriate state to be able to query with those existing functions).

Note: These tests fully pass with Pester 4.10.1. Some additional work may be done prior to merging to get them passing with Pester 5.0, but the priority is to get this merged into master soon in order to unblock the pipeline of existing pull requests that have been waiting on this change.

Issues Fixed

References

The whole API set

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

…ormation to be more natural

  (and to work better with pipelining)

* Added new pipeline-related tests to GitHubAssinees.tests.ps1

* Created GitHubUsers.tests.ps1 and added a number of new tests for Users.

* Moved a number of unrelated tests out of GitHubAnalytics.tests.ps1 to existing or new files:
   * Some tests moved to GitHubRepositories.tests.ps1
   * Moved `Split-GitHubUri` tests to GitHubCore.tests.ps1
      * Also added some new ones
   * Some tests moved to a new GitHubBranches.tests.ps1
   * Some tests moved to a new GitHubIssues.tests.ps1
* Fixed handling of typing for Repo Contributors, Collaborators and Tags.
* Added missing 'Affiliation' parameter to Get-GitHubRepositoryCollaborator
* Completed required repository tests
@HowardWolosky HowardWolosky added the enhancement An issue or pull request introducing new functionality to the project. label Jun 17, 2020
@HowardWolosky
Copy link
Member Author

@X-Guardian / @rjmholt / @TylerLeonhardt / @themilanfan - I'd be interested in your input. The change here is pretty robust and well-tested, but it would be great to get some other eyes on this for how it feels. If you have a chance, try checking out the branch and running through some scenarios to see if it's working the way that you'd expect. I'd love any feedback.

I really want to get this change merged in before taking in much of the outstanding PR's that are adding new functionality.

@HowardWolosky
Copy link
Member Author

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky HowardWolosky changed the title Pipelining Adding complete Pipeline support to the entire module Jun 17, 2020
@HowardWolosky HowardWolosky added the breaking change This includes a change in existing published module behavior. label Jun 17, 2020
Copy link
Contributor

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

Great work @HowardWolosky , looks good. I've tested passing GitHub.Repo objects between functions, and everything I tried worked fine.

I've reviewed all the module files, but not looked at the test files and beyond yet.

CONTRIBUTING.md Show resolved Hide resolved
GitHubBranches.ps1 Show resolved Hide resolved
GitHubBranches.ps1 Outdated Show resolved Hide resolved
GitHubIssueComments.ps1 Show resolved Hide resolved
GitHubIssueComments.ps1 Outdated Show resolved Hide resolved
GitHubRepositories.ps1 Outdated Show resolved Hide resolved
GitHubRepositories.ps1 Show resolved Hide resolved
GitHubRepositories.ps1 Show resolved Hide resolved
GitHubRepositoryTraffic.ps1 Outdated Show resolved Hide resolved
GitHubRepositoryTraffic.ps1 Outdated Show resolved Hide resolved
@X-Guardian
Copy link
Contributor

Also, how about adding the supported pipeline input object types to .INPUTS on the comment-based help for each function?

@HowardWolosky
Copy link
Member Author

Great work @HowardWolosky Howard Wolosky FTE , looks good. I've tested passing GitHub.Repo objects between functions, and everything I tried worked fine.

Thanks -- and to be clear (if it wasn't already), this isn't limited to repo objects. You could even pipe a milestone object into New-GitHubIssue to create a new issue against that milestone, or you could pipe an issue object into Get-GitHubRepository to get info on its parent repo, etc... There are a ton of really useful pipe combinations that make things so much cleaner and more powerful.

A simple repo one that I've enjoyed lately on my test account when I was doing a lot of verification against the work was:

Get-GitHubRepository | Remove-GitHubRepository -Force

So clean and clear.

Thanks for the review/feedback as well, @X-Guardian ... much appreciated.

@HowardWolosky
Copy link
Member Author

Also, how about adding the supported pipeline input object types to .INPUTS on the comment-based help for each function?

That's reasonable feedback. It was an oversight because I wasn't familiar enough with what it was supposed to specifically address, but I just looked it up. I'll get the specific GitHub.* types that are pipelinable mentioned in the .INPUTS for each function.

@HowardWolosky
Copy link
Member Author

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky HowardWolosky merged commit 17f6122 into microsoft:master Jun 18, 2020
HowardWolosky added a commit to HowardWolosky/PowerShellForGitHub that referenced this pull request Jun 28, 2020
* Fixes a regression that was introduced in microsoft#242.  A `Write-Log`
  command was accidentally updated to pass in `=Message` instead of
  `-Message`.
* Updates the web request to suppress the progress bar using
  $ProgressPreference = 'SilentlyContinue'

Fixes microsoft#249
Fixes microsoft#250
HowardWolosky added a commit that referenced this pull request Jun 28, 2020
* Fixes a regression [introduced by #242 unfortunately](17f6122#diff-34a614678760cc83c5a91ad95ae240e5R86) when I was reducing the lines in the module that exceeded 100 chars.

Specifically (note the `=Message` instead of the `-Message`):
https://github.com/microsoft/PowerShellForGitHub/blob/17f6122d7812ee4001ce4bdf630429e711e45f7b/UpdateCheck.ps1#L86

* Updates the web request to suppress the progress bar using `$ProgressPreference = 'SilentlyContinue'`

* Adds a `-Force` switch to force the update check to happen again to make future debugging scenarios easier.

Fixes #249
Fixes #250
@HowardWolosky HowardWolosky deleted the pipelining branch June 30, 2020 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This includes a change in existing published module behavior. enhancement An issue or pull request introducing new functionality to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support seamless pipelining of related cmdlets
2 participants