Updates to Update-Changelog.ps1 #1299
Conversation
chidozieononiwu
commented
Jan 4, 2021
•
chidozieononiwu
commented
- This makes sure most of behavior is same as eng\common\Update-Change-Log.ps1 with better handling for out side of normal use cases.
|
The following pipelines have been queued for testing: |
chidozieononiwu
force-pushed
the
UpdateUpdateChangeLog
branch
from
befaef0 to
31a173c
Compare
|
The following pipelines have been queued for testing: |
| [String]$ReleaseDate | ||
| ) | ||
|
|
||
| if ($ReleaseDate -and ($Unreleased -eq $True)) { | ||
| [Boolean]$Unreleased = [System.Convert]::ToBoolean($Unreleased) |
There was a problem hiding this comment.
Why do the manual conversion here as opposed to just having the parameters be Boolean as they currently are?
There was a problem hiding this comment.
We call it from various python and Js scripts. Passing the parameters a Booleans was problematic. It wasn't clear to me how to get it working from all ends with Boolean parameters hence I just did the conversion after.
There was a problem hiding this comment.
I figured you could just pass in the string "true" or "false" (or 0/1) as arguments and it would just work. Did that not work when calling it from python/JS?
There was a problem hiding this comment.
Yes that the issue. Even thought "true" and "false" is passed in the JS script . It is still recognized a s string on the PowerShell side and errors if the parameters are Boolean. Python has a similar weirdness.
There was a problem hiding this comment.
OK. We could also consider making these switches so we don't have to pass a value in most cases. In either case if you keep these as strings it would be nice to add a comment as to why you are doing it so it is clear to use later.
| { | ||
| $newChangeLogEntry = New-ChangeLogEntry -Version $Version -Status $ReleaseStatus -Content $ChangeLogEntries[$LatestVersion].ReleaseContent | ||
| LogDebug "Resetting latest entry title to [$($newChangeLogEntry.ReleaseTitle)]" |
There was a problem hiding this comment.
Why LogDebug here as opposed to a standard log message?
There was a problem hiding this comment.
I could make it the standard output. Just the LogDebug is colored blue on DevOps which looks quite better, that's the only reason am using it here.
There was a problem hiding this comment.
Ok I guess LogDebug just didn't seem right in my head as I associated it with Write-Debug which will not write anything unless Debug preference is setup. We may want to think about the naming that function a little more.
| } | ||
| } | ||
| elseif ($ChangeLogEntries.Contains($Version)) | ||
| { | ||
| $ChangeLogEntries[$Version].ReleaseVersion = $Version |
There was a problem hiding this comment.
Do you know if there was any reason why ReleaseVersion wouldn't match Version? Did we used to set it when we didn't need to?
There was a problem hiding this comment.
The ReleaseVersion should be the same as the Version in the title of the Release Entry. Its just that its named ReleaseVersion in the object returned from the ChangeLog operation.
There was a problem hiding this comment.
Ok if there is no case were we need to update it then I'm fine with removing this.
chidozieononiwu
force-pushed
the
UpdateUpdateChangeLog
branch
from
31a173c to
a710575
Compare
|
The following pipelines have been queued for testing: |
|
Hello @azure-sdk! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit
| @@ -12,41 +12,46 @@ param ( | |||
| [String]$ServiceDirectory, | |||
| [Parameter(Mandatory = $true)] | |||
| [String]$PackageName, | |||
| [boolean]$Unreleased=$True, | |||
| [boolean]$ReplaceLatestEntry = $False, | |||
| [String]$Unreleased=$True, | |||
There was a problem hiding this comment.
If you want these to be strings now we should probably default them to strings instead of booleans.
