-
Notifications
You must be signed in to change notification settings - Fork 513
Add scripts for automatically updating the version of the PowerShell extension #1932
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
Conversation
Example ADS PR: microsoft/azuredatastudio#5247 Example PSES PR: rjmholt/PowerShellEditorServices#4 Example vscode-PowerShell PR: rjmholt#6 |
One thing I'm ambivalent about is the PSES release script. It more suitably belongs in the PSES repo, but it uses the modules in this repo. In the PowerShell repo there are build steps that download GitHub assets and it makes the build very hard to follow, reason about, debug or change and I wanted to avoid that. But philosophically PSES is its own thing. Since we currently always release both together, I can see justification for keeping all of it in vscode-PowerShell for now, especially since it simplifies things. But I don't entirely like it either. |
What's the reasoning for the version update scripts? So we just need to run that instead of manually sending a PR to rev the versions after a release? |
|
||
$repoLocation = Join-Path ([System.IO.Path]::GetTempPath()) 'ads-temp-checkout' | ||
|
||
if (-not $BranchName) |
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.
Can you put this in the default parameters?
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.
It depends on another parameter. Does that work?
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.
Ah, experimenting shows this depends on the order of parameters
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.
Turns out there was complicated file-reading logic that needed to be done when $NewVersion
wasn't provided, so I had to move this back out
tools/GitHubTools.psm1
Outdated
|
||
[Parameter(Mandatory)] | ||
[string] | ||
$Destination, |
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.
maybe this could just have a default of ""
or the current directory... or some random file directory.
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.
""
would just error I think.
I tried to get a sensible default here, which would be the repo name, but it turns out that can be complicated
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.
So I decided to leave it as mandatory for now, let the human figure it out
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.
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 I have a thing that works I think
Co-Authored-By: rjmholt <[email protected]>
Yeah, just another component in being able to press the release button and having everything happen that needs to happen; automating the mindless without loss of oversight. |
tools/FileUpdateTools.psm1
Outdated
return $stringBuilder.ToString().Replace([System.Environment]::NewLine, "`r`n") | ||
} | ||
|
||
function SetFileContent |
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 not use Set-Content
?
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.
Was initially to work around the UTF8 BOM thing, but I think you're right that Set-Content is best
$MainTsContent | ||
) | ||
|
||
$pattern = [regex]'const\s+requiredEditorServicesVersion\s+=\s+"(.*)"' |
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'm gonna be honest... I don't think we actually use that variable anywhere...
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 doubt it too, but while it's in the code I don't want to let it rot
SetFileContent -FilePath $DockerFilePath -Value $newDockerFileContent | ||
} | ||
|
||
function GetMarketplaceVersionFromSemVer |
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.
This looks similar to IncrementVersion
- maybe worth combining somehow?
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.
No, it converts a SemVer v2 version to what we've been using in the marketplace
Co-Authored-By: rjmholt <[email protected]>
Co-Authored-By: rjmholt <[email protected]>
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.
LGTM + a few nits
|
||
# Write out the new entry | ||
SetFileContent $GalleryFilePath $newGalleryFileContent | ||
Set-Content -Path $GalleryFilePath -Value $newGalleryFileContent -Encoding utf8NoBOM -NoNewline |
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 think this means it'll only work on PS6 (utf8noBOM
) - which I'm ok with for this script, but wanted to call this out
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.
Yeah, that's right
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.
Basically the semver handling means this will only work in PS 6, so with that in mind I've gone with a few PS6-specific things, since we can always opt for pwsh in AzDO (and unlike building, it's not important to be able to run release code in WinPS-only contexts)
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.
Should probably add this then:
#Requires -Version 6
New-GitHubPR @prParams |
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.
nit, newline
$newDockerFileContent = ReplaceStringSegment -String $vstsDockerFileContent -NewSegment $Version -StartIndex $vstsDockerFileVersionSpan.Start -EndIndex $vstsDockerFileVersionSpan.End | ||
SetFileContent -FilePath $DockerFilePath -Value $newDockerFileContent | ||
$newDockerFileContent = New-StringWithSegment -String $vstsDockerFileContent -NewSegment $Version -StartIndex $vstsDockerFileVersionSpan.Start -EndIndex $vstsDockerFileVersionSpan.End | ||
Set-Content -Path $DockerFilePath -Value $newDockerFileContent -Encoding utf8NoBOM -NoNewline |
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.
Same comment on "requiring PS6"
New-GitHubPR @prParams |
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.
nit, newline
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.
LGTM go ahead and do the rebase in the other PR! 👍
PR Summary
Adds scripts for:
PR Checklist
WIP:
to the beginning of the title and remove the prefix when the PR is ready