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

Add support for repository autolink #371

Closed
wants to merge 26 commits into from

Conversation

henkmeulekamp
Copy link

Description

Implement basic support for autolinks (s) on a repository.

I'm quite new to writing PowerShell Modules, let me know If I missed something.

Issues Fixed

References

https://docs.github.com/en/rest/repos/autolinks#create-an-autolink-reference-for-a-repository

{
"id": 1,
"key_prefix": "TICKET-",
"url_template": "https://example.com/TICKET?query=",
"is_alphanumeric": true
}

Get-GitHubRepositoryAutoLink  [-OwnerName <String>] [-RepositoryName <String>] [-AccessToken <String>]

New-GitHubRepositoryAutoLink  [-OwnerName <String>] [-RepositoryName <String>] [-AccessToken <String>] 
  [-KeyPrefix <String>]  [-UrlTemplate <String>]  [-IsNumeric] 

Remove-GitHubRepositoryAutoLink  [-OwnerName <String>] [-RepositoryName <String>] [-AccessToken <String>] 
  [-AutoLinkId] <Int64> [-Force] [-WhatIf] [-Confirm]

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

@henkmeulekamp henkmeulekamp changed the title Feature/autolink #368 Add support for repository autolink #368 Oct 4, 2022
@henkmeulekamp henkmeulekamp changed the title Add support for repository autolink #368 Add support for repository autolink Oct 4, 2022
@henkmeulekamp
Copy link
Author

@microsoft-github-policy-service agree

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 the contribution, @henkmeulekamp! Impressive job for your first attempt at writing some PowerShell code.

It looks like there's still some work to be done in here before it can be merged, but you're very far along on the journey here.

Looking forward to seeing the next update.

@@ -7,7 +7,7 @@
CompanyName = 'Microsoft Corporation'
Copyright = 'Copyright (C) Microsoft Corporation. All rights reserved.'

ModuleVersion = '0.16.1'
ModuleVersion = '0.17.0'
Copy link
Member

Choose a reason for hiding this comment

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

Please don't modify the version with your change. This only happens when a new module version is being released.

USAGE.md Outdated

#### Create a new autolink
```powershell
New-GitHubRepositoryAutolink -OwnerName microsoft -RepositoryName PowerShellForGitHub -UriPrefix PRJ- -Urltemplate https://company.issuetracker.com/browse/prj-<num>
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
New-GitHubRepositoryAutolink -OwnerName microsoft -RepositoryName PowerShellForGitHub -UriPrefix PRJ- -Urltemplate https://company.issuetracker.com/browse/prj-<num>
New-GitHubRepositoryAutolink -OwnerName microsoft -RepositoryName PowerShellForGitHub -KeyPrefix 'PRJ-' -UrlTemplate 'https://company.issuetracker.com/browse/prj-<num>'

.OUTPUTS
GitHub.RepositoryAutolink

.EXAMPLE
Copy link
Member

Choose a reason for hiding this comment

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

Please add a .NOTES that this is restricted to repository admins only.
Example:

.NOTES
Protecting a branch requires admin or owner permissions to the repository.

Please add to all of these commands.

The OwnerName and RepositoryName will be extracted from here instead of needing to provide
them individually.

.PARAMETER Sort
Copy link
Member

Choose a reason for hiding this comment

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

Not relevant for this API. Copy/paste error?

[Alias('RepositoryUrl')]
[string] $Uri,

[ValidateSet('Newest', 'Oldest', 'Stargazers')]
Copy link
Member

Choose a reason for hiding this comment

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

Not relevant for this API. Copy/paste error?

Add-Member -InputObject $item -Name 'AutolinkId' -Value $item.id -MemberType NoteProperty -Force
}

Add-Member -InputObject $item -Name 'AutolinkKeyPrefix' -Value $item.key_prefix -MemberType NoteProperty -Force
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
Add-Member -InputObject $item -Name 'AutolinkKeyPrefix' -Value $item.key_prefix -MemberType NoteProperty -Force
Add-Member -InputObject $item -Name 'KeyPrefix' -Value $item.key_prefix -MemberType NoteProperty -Force

}

Add-Member -InputObject $item -Name 'AutolinkKeyPrefix' -Value $item.key_prefix -MemberType NoteProperty -Force
Add-Member -InputObject $item -Name 'AutolinkUrlTemplate' -Value $item.url_template -MemberType NoteProperty -Force
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
Add-Member -InputObject $item -Name 'AutolinkUrlTemplate' -Value $item.url_template -MemberType NoteProperty -Force
Add-Member -InputObject $item -Name 'UrlTemplate' -Value $item.url_template -MemberType NoteProperty -Force

Comment on lines 21 to 27
@{
defaultRepoDesc = "This is a description."
defaultRepoHomePage = "https://www.microsoft.com/"
defaultRepoTopic = "microsoft"
}.GetEnumerator() | ForEach-Object {
Set-Variable -Force -Scope Script -Option ReadOnly -Visibility Private -Name $_.Key -Value $_.Value
}
Copy link
Member

Choose a reason for hiding this comment

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

Where are these being used?

}

It 'Should have the expected type and additional properties' {
$team.PSObject.TypeNames[0] | Should -Be 'GitHub.RepositoryAutolink'
Copy link
Member

Choose a reason for hiding this comment

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

This makes me feel like you didn't validate your tests before submitting the PR since this is referencing both a variable ($team) and properties (like OranizationName) that don't exist.

Set-Variable -Force -Scope Script -Option ReadOnly -Visibility Private -Name $_.Key -Value $_.Value
}

Describe 'GitHubRepositoryAutolink\Get-GitHubRepositoryAutolink' {
Copy link
Member

Choose a reason for hiding this comment

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

This is a great first test, but you have a few more to add:

  • Retrieving a single autolink
  • Creating an autolink via the pipeline
  • etc...

Refer to some of the other test files to get an idea on the breadth of scenarios covered.

Copy link
Member

Choose a reason for hiding this comment

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

Please also take a look at the work being done in #366, and adjust your test file to be Pester 5 compliant.

@ghost ghost added the needs-author-feedback label Oct 6, 2022
@ghost ghost removed the needs-author-feedback label Oct 6, 2022
@henkmeulekamp henkmeulekamp marked this pull request as draft October 6, 2022 20:46
@henkmeulekamp
Copy link
Author

Resolved most of the comments, many thanks for bringing these up. I missed some obvious things on the first run.. maybe due to focusing to much on PowerShell working itself.

Will have another look at the tests and fix the 2 todo comments.

Many thanks for the review so far. Learning a lot of PowerShell constructs/wow. Will resubmit PR after addressing the last open points.

- num block is important and requires validation
@microsoft-github-policy-service
Copy link
Contributor

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 14 days of this comment.

@microsoft-github-policy-service microsoft-github-policy-service bot added the auto-closed-unmerged An unmerged pull request that has been auto-closed due to lack of activity. label Aug 13, 2023
@microsoft-github-policy-service
Copy link
Contributor

This pull request has been automatically closed due to a lack of activity from the author. We understand. Life happens and other things likely came up. We would still love to see your contribution get merged in. Now that it has been closed, a different community member may wish to pick up where you left off. If so, they should speak up by commenting below. If you're still interested in completing this yourself, just respond back and let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-closed-unmerged An unmerged pull request that has been auto-closed due to lack of activity. needs-author-feedback status-no-recent-activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants