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

GitHubRepositories: Add New-GitHubRepositoryFromTemplate Function #221

Merged
merged 18 commits into from
Jun 26, 2020
Merged

GitHubRepositories: Add New-GitHubRepositoryFromTemplate Function #221

merged 18 commits into from
Jun 26, 2020

Conversation

X-Guardian
Copy link
Contributor

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

Description

This PR adds a new function New-GitHubRepositoryFromTemplate to the GitHubRepositories module. This function creates a new GitHub repository from a specified template repository.

Parameters
Name Type Description
RepositoryName Mandatory String Name of the repository to be created.
TemplateOwnerName Mandatory String Owner of the template repository.
TemplateRepositoryName Mandatory String Name of the template repository.
OwnerName String Owner of the repository to be created. If not specified, the DefaultOwnerName configuration property value will be used.
Description String A short description of the repository.
Private Switch By default, this repository will be created Public. Specify this to create a private repository.
AccessToken String If provided, this will be used as the AccessToken for authentication with the REST Api. Otherwise, will attempt to use the configured value or will run unauthenticated.
NoStatus Switch If this switch is specified, long-running commands will run on the main thread with no commandline status update. When not specified, those commands run in the background, enabling the command prompt to provide status information. If not supplied here, the DefaultNoStatus configuration property value will be used.

Issues Fixed

References

Create a repository using a template

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
Copy link
Contributor Author

Can we trigger the CI for this PR?

@HowardWolosky
Copy link
Member

I'll kick off a CI build a little bit later today after the current one ends.

@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

Can we trigger 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

A weird CI failure on MacOS for the static code analysis step:

   2 |  $results = Invoke-ScriptAnalyzer -Path ./ –Recurse
     |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Collection was modified; enumeration operation may not execute.

May be related to this: Collection was modified; enumeration operation may not execute when running Invoke-ScriptAnalyzer

can you run the CI again and see if it is consistent?

@HowardWolosky
Copy link
Member

Yeah, I've been seeing that pop up from time to time. I've just been waiting out a fix for that bug.

@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@X-Guardian X-Guardian marked this pull request as ready for review June 12, 2020 22:53
@bergmeister
Copy link
Contributor

bergmeister commented Jun 13, 2020

@HowardWolosky Can you please open a new issue in the PSScriptAnalyzer repo with steps to repro: https://github.com/PowerShell/PowerShell/issues/new?assignees=&labels=Issue-Question&template=Bug_Report.md&title=My+bug+report
The referenced bug PowerShell/PSScriptAnalyzer#1516 that you mention is specific to the new 1.19.0 release and only when using the PSUseCorrectCasing rule that is not run by default by Invoke-ScriptAnalyzer. Therefore your issue is likely a completely different one despite the error message being the same. I cloned this branch locally and cannot reproduce unfortunately :-(
I opened PR #236 to help you log and report the error the next time.

HowardWolosky pushed a commit that referenced this pull request Jun 15, 2020
…rove ScriptAnalyzer installation to not skip publisher and check and not allow clobber (#236)

#221 reported that this repo causes PSScriptAnalyzer to sporadically throw an error.
In order to report the full error needed for analysis, report the full stack trace. In order to make this easier, I've updated `Invoke-ScriptAnalyzer` to run within a `try/catch using -ErrorAction Stop`, and report any error found with `$_.Exception.StackTrace` before re-throwing the error.
@X-Guardian
Copy link
Contributor Author

Can we trigger the CI for this PR?

@HowardWolosky HowardWolosky added api-repositories Work to complete the API's defined here: https://developer.github.com/v3/repos/ 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
Copy link
Member

Can we trigger the CI for this PR?

I can once the conflicts stemming from #242 have been resolved.

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 for adding this functionality!

Some feedback is included for you to review.

GitHubRepositories.ps1 Show resolved Hide resolved
GitHubRepositories.ps1 Outdated Show resolved Hide resolved
GitHubRepositories.ps1 Show resolved Hide resolved
GitHubRepositories.ps1 Show resolved Hide resolved
GitHubRepositories.ps1 Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubRepositories.tests.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 Jun 20, 2020
@X-Guardian
Copy link
Contributor Author

Can we trigger 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).

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.

Super minor additional changes, and then we can get this merged. Thanks for the new feature!

GitHubRepositories.ps1 Outdated Show resolved Hide resolved
GitHubRepositories.ps1 Outdated Show resolved Hide resolved
GitHubRepositories.ps1 Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Show resolved Hide resolved
Tests/GitHubRepositories.tests.ps1 Show resolved Hide resolved
USAGE.md Outdated Show resolved Hide resolved
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.

Just noticed one bug before I was about to run CI. I'll kick off CI and then merge this in once that gets fixed.

GitHubRepositories.ps1 Outdated Show resolved Hide resolved
GitHubRepositories.ps1 Outdated Show resolved Hide resolved
@X-Guardian
Copy link
Contributor Author

@HowardWolosky, ready for 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 , can we trigger the CI for this PR?

@HowardWolosky
Copy link
Member

Sure thing. Will kick it off once the current one completes, and then get this merged in.

@HowardWolosky HowardWolosky removed the waiting for update Waiting for an update to the PR before the next review label Jun 26, 2020
@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@HowardWolosky HowardWolosky merged commit d96541e into microsoft:master Jun 26, 2020
@HowardWolosky
Copy link
Member

@X-Guardian - looks like this work doesn't fully support pipeline input, and the tests aren't sufficiently figuring that out either.

You're not capturing $RepositoryName = $elements.repositoryName at the top, which means that input from $Uri will never resolve correctly.

$elements = Resolve-RepositoryElements -BoundParameters $PSBoundParameters
$OwnerName = $elements.ownerName
$telemetryProperties = @{

  1. You're passing in -WhatIf which meant that you missed the bug in the method, since the failure was only seen as an exception thrown from the GitHub API itself when RepositoryName was missing in the uri fragment itself which won't occur due to the -WhatIf (the pipeline input needs to be a real separate test):

{ $templateRepo | New-GitHubRepositoryFromTemplate @newGitHubRepositoryFromTemplateParms -WhatIf } |
Should -Not -Throw

  1. Once you make those changes, I'm seeing some additional failures that start to occur.

Want to take a crack at a fix?

HowardWolosky added a commit to HowardWolosky/PowerShellForGitHub that referenced this pull request Jun 30, 2020
Added in microsoft#221, `New-GitHubRepositoryFromTemplate was not capturing
the passed-in value for the RepositoryName if provided via a Uri
parameter.

There was a pipeline test in place, however it missed this fact
because it was only testing the pipeline input scenario using `-WhatIf`,
and the error was only found when calling the actual REST API and
GitHub noticed that the `RepositoryName` was missing from the URI.
@HowardWolosky
Copy link
Member

I put out a fix with #259

HowardWolosky added a commit that referenced this pull request Jun 30, 2020
…#259)

Added in #221, `New-GitHubRepositoryFromTemplate was not capturing the passed-in value for the RepositoryName if provided via a Uri parameter.

There was a pipeline test in place, however it missed this fact because it was only testing the pipeline input scenario using `-WhatIf`, and the error was only found when calling the actual REST API and GitHub noticed that the `RepositoryName` was missing from the URI.
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-repositories Work to complete the API's defined here: https://developer.github.com/v3/repos/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Function Proposal: New-GitHubRepositoryFromTemplate
3 participants