-
Notifications
You must be signed in to change notification settings - Fork 183
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
Update Changelog Logic to accommodate older entries #1335
Conversation
@@ -2,7 +2,7 @@ | |||
. "${PSScriptRoot}\logging.ps1" | |||
. "${PSScriptRoot}\SemVer.ps1" | |||
|
|||
$RELEASE_TITLE_REGEX = "(?<releaseNoteTitle>^\#+.*(?<version>\b\d+\.\d+\.\d+([^0-9\s][^\s:]+)?)(\s+(?<releaseStatus>\(Unreleased\)|\(\d{4}-\d{2}-\d{2}\)))?)" | |||
$RELEASE_TITLE_REGEX = "(?<releaseNoteTitle>^\#+.*(?<version>\b\d+\.\d+\.\d+([^0-9\s][^\s:]+)?)(\s+(?<releaseStatus>\(.*\))?)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why loosen this? If we don't match the title then can we just leave the entry alone?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will be using it in other places though. e.g. in collect changelog to pull out released for the required date. I guess we have to enforce it at some point to make sure its valid date format. We also loosened it for changelog verification before release. But we can just loosen it here and then if we need to make use of the date we can error and indicate the date is wrong then it can be fixed on the changelog itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were depending on the regex to enforce our guidelines but if you loosen it here then we should add extra validation in other places in the helper scripts to ensure people still follow the guidelines.
However I'm not sure why we cannot simply ignore any sections that aren't in the valid format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know of any other place we are using this regex. Looks like its only used to grab the release status/date. We do enforce that its a valid date before releasing
[DateTime]$status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok lets see if this helps with the older entries. How do we plan to handle the version numbers with preview but no number?
The following pipelines have been queued for testing: |
cba9894
to
27f3f8a
Compare
The following pipelines have been queued for testing: |
27f3f8a
to
ea2a808
Compare
The following pipelines have been queued for testing: |
@@ -191,16 +191,7 @@ function Set-ChangeLogContent { | |||
$changeLogContent += "# Release History" | |||
$changeLogContent += "" | |||
|
|||
try | |||
{ | |||
$VersionsSorted = [AzureEngSemanticVersion]::SortVersionStrings($ChangeLogEntries.Keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK this is is what removed some of the entries that didn't match? So we no longer sort the entries at all then? That might be easier but I just want to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we were sorting it in three places. I removed two of them. We still do one sorting here. I left that one because it shouldn't be too much of a problem, am just using it to see if an older version than the latest is being used for the update.
) | ||
|
||
$results = [Ordered]@{} | ||
$results.Add($Version, $NewChangeLogEntry) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming that this change will resolve storage version increment issue I have mentioned earlier so PR #1331 is not required. right? |
@praveenkuttappan yes I closed your PR in favor of this one. |
The following pipelines have been queued for testing: |
da2862d
to
b2809e3
Compare
The following pipelines have been queued for testing: |
b2809e3
to
39b4485
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 (
|
This change is to accommodate parts of the changelog that does not properly indicate the release status as specified in the changelog guideline.
This also switched the logic to use Ordered HashTable so as not to rely on SemVer sorting. This allows us accommodate version that are not SemVer compliant.