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 Get-GitHubRelease #125

Merged
merged 4 commits into from
Sep 18, 2019
Merged

Adding Get-GitHubRelease #125

merged 4 commits into from
Sep 18, 2019

Conversation

smaglio81
Copy link
Contributor

Similar to #110, I needed some functionality around releases. I didn't so much need functionality to publish a release, but I needed to retrieve the list of releases that we're published. So, this is just a function for Get-GitHubRelease.

@msftclas
Copy link

msftclas commented Jul 26, 2019

CLA assistant check
All CLA requirements met.

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 so much for this contribution. And those tests! Well done. Some minor requested changes, and then we should be good to merge in.

Tests/GitHubReleases.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubReleases.tests.ps1 Outdated Show resolved Hide resolved
Tests/GitHubReleases.tests.ps1 Outdated Show resolved Hide resolved
$releases = Get-GitHubRelease -OwnerName $ownerName -RepositoryName $repositoryName

Context 'When getting all releases' {

Copy link
Member

Choose a reason for hiding this comment

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

nit: Please remove this empty line.

Tests/GitHubReleases.tests.ps1 Outdated Show resolved Hide resolved
GitHubReleases.ps1 Outdated Show resolved Hide resolved

if(-not [String]::IsNullOrEmpty($Tag))
{
$telemetryProperties['Tag'] = Get-PiiSafeString -PlainText $Tag
Copy link
Member

@HowardWolosky HowardWolosky Jul 29, 2019

Choose a reason for hiding this comment

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

Suggested change
$telemetryProperties['Tag'] = Get-PiiSafeString -PlainText $Tag
$telemetryProperties['ProvidedTag'] = $true
``` #Resolved

GitHubReleases.ps1 Outdated Show resolved Hide resolved
$telemetryProperties['Latest'] = Get-PiiSafeString -PlainText "latest"

$uriFragment += "/latest"
$description = "Getting releases for $OwnerName/$RepositoryName/releases/latest"
Copy link
Member

@HowardWolosky HowardWolosky Jul 29, 2019

Choose a reason for hiding this comment

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

Suggested change
$description = "Getting releases for $OwnerName/$RepositoryName/releases/latest"
$description = "Getting latest release for $OwnerName/$RepositoryName"
``` #Resolved

GitHubReleases.ps1 Show resolved Hide resolved
@HowardWolosky
Copy link
Member

/azp run PowerShellForGitHub-CI

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@smaglio81
Copy link
Contributor Author

I've tried to incorporate all the changes that you've suggested. But, I'm pretty new to git and even newer to github. I've updated the code base There's an updated version of the code at master...ucsb:master. Should I submit a new pull request?

@HowardWolosky
Copy link
Member

Should I submit a new pull request?

No need to submit a new PR. We just continue with this one. Sorry for not responding sooner. I missed your comments from Sunday. I'll take a look at all of the updates soon (within the next few days).

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.

Great updates.
Very minor changes requested to your examples, and then this should be good for merging. Thanks for doing this work!

GitHubReleases.ps1 Outdated Show resolved Hide resolved
GitHubReleases.ps1 Outdated Show resolved Hide resolved
@smaglio81
Copy link
Contributor Author

So ... I kept trying to use the "Commit Suggestion" button on the web interface, and I must have been doing something wrong because it just wouldn't commit.

Anyways, I went to the source and made the "owner" --> "default configured" changes and have updated my github repo.

Give it another perusal when you have a chance. Thanks!

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.

This looks great!
Thanks so much for adding this feature.
Congrats on getting in your first change!
I'll publish out a new release in the next couple days that includes this change (I'm currently working on getting that publishing step fully automated).

@HowardWolosky HowardWolosky self-assigned this Aug 23, 2019
@smaglio81
Copy link
Contributor Author

Thanks!

I feel bad asking this, but I was wondering if you’ve had a moment to complete this merge?

@HowardWolosky
Copy link
Member

I feel bad asking this, but I was wondering if you’ve had a moment to complete this merge?

Sorry for the delay, @smaglio81. I'm mid-process of automating the whole release pipeline, and the merging/publishing of this change will be used to validate that new automation. So, expect it to be merged in this week. Thanks for your patience!

@HowardWolosky HowardWolosky merged commit 7ea773c into microsoft:master Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants