-
Notifications
You must be signed in to change notification settings - Fork 244
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
Remove reference to removed VersionCheckFile #1481
base: main
Are you sure you want to change the base?
Conversation
@twneale Testing section should also include how the reviewers can test the code. Also, have you tested it when running from the ZIP to see if it's fully resolved there too? |
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.
The removal of the test code is perfect. But while reviewing I had some other suggestions and a possible bug that requires more extensive changes. See comments below.
Improved verbiage Co-authored-by: Addam Schroll <[email protected]>
@schrolla what would you think about checking PSGallery and comparing it to the module manifest version of the running ScubaGear rather than querying the installed versions and getting the latest? This way it would still work when running as a git checkout. Otherwise, running from a latest git checkout can result in an inaccurate warning to "upgrade" to a lower version. Instead of trying to guess whether it was installed from PSGallery or a GitHub zip, we could just warn that newer releases are available from both PSGallery and Github. Watcha think? |
Yes, I think that's a reasonable way of checking since checking the manifest based on the PSScriptRoot of the running version check script makes it a direct comparison between the published available version and the running version. Whether you check GitHub or PSGallery for the comparison it doesn't much matter as they should be the same and are both official. I'd slightly prefer checking GitHub as that's the official release and PSGallery is just our package distribution mechanism which could (although not likely for now) change over time. |
@schrolla I made that change and the code is short and sweet now. I added instructions for testing it out. I think it may be ready to rip now. |
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.
See minor typo and wording suggestions and one possible TODO follow-on issue.
|
||
function Invoke-CheckScubaGearVersionGithub { | ||
.DESCRIPTION | ||
Checks latest version available on the github release page and compares it to the current running verison. |
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.
Typo and capitalization fix
Checks latest version available on the github release page and compares it to the current running verison. | |
Checks latest version available on the Github release page and compares it to the current running version. |
if ($CurrentVersion -ne $LatestVersion) { | ||
$CurrentVersion = [System.Version]$ScubaManifest.ModuleVersion | ||
$LatestVersion = [System.Version]$(Invoke-RestMethod -Uri "https://api.github.com/repos/cisagov/ScubaGear/releases/latest" -ErrorAction 'Stop').tag_name.TrimStart("v") | ||
if ($CurrentVersion -lt $LatestVersion) { | ||
Write-Warning "A new version of ScubaGear ($latestVersion) is available. Please consider updating at: https://github.com/cisagov/ScubaGear/releases. This notification can be disabled by setting `$env:SCUBAGEAR_SKIP_VERSION_CHECK = `$true before running ScubaGear." |
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.
The latest version may be comparatively newer without being new. Suggest updating language.
Write-Warning "A new version of ScubaGear ($latestVersion) is available. Please consider updating at: https://github.com/cisagov/ScubaGear/releases. This notification can be disabled by setting `$env:SCUBAGEAR_SKIP_VERSION_CHECK = `$true before running ScubaGear." | |
Write-Warning "A newer version of ScubaGear ($latestVersion) is available. Please consider updating at: https://github.com/cisagov/ScubaGear/releases. This notification can be disabled by setting `$env:SCUBAGEAR_SKIP_VERSION_CHECK = `$true before running ScubaGear." |
if ($CurrentVersion -ne $LatestVersion) { | ||
$CurrentVersion = [System.Version]$ScubaManifest.ModuleVersion | ||
$LatestVersion = [System.Version]$(Invoke-RestMethod -Uri "https://api.github.com/repos/cisagov/ScubaGear/releases/latest" -ErrorAction 'Stop').tag_name.TrimStart("v") | ||
if ($CurrentVersion -lt $LatestVersion) { | ||
Write-Warning "A new version of ScubaGear ($latestVersion) is available. Please consider updating at: https://github.com/cisagov/ScubaGear/releases. This notification can be disabled by setting `$env:SCUBAGEAR_SKIP_VERSION_CHECK = `$true before running ScubaGear." |
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.
Thinking about where we point them when a new version is available. Do we point directly to the GitHub release, or to our documentation? PSGallery users won't get much from the GitHub link.
We have install instructions, but not specifically upgrade instructions. Might be a good section to add and maintain in case we run into any difficult/breaking change upgrade paths. Already an issue in #1223. So maybe add to that issue that once instructions are available, update version update function to point to them instead.
Fix Version Check Path Bug
The version check code has a reference to a removed variable still present. This PR deletes it and closes #1480
🗣 Description
Deleted two lines of code writing out the version check file, which was removed for Kraken.
💭 Motivation and context
Currently this bug causes a error and stack trace on import.
Closes #1480
🧪 Testing
Alternatively, you can just decrement the ModuleVersion in the scuba manifest and import from the git branch to view the warning.
✅ Pre-approval checklist
✅ Pre-merge checklist
PR passed smoke test check.
Feature branch has been rebased against changes from parent branch, as needed
Use
Rebase branch
button below or use this reference to rebase from the command line.Resolved all merge conflicts on branch
Notified merge coordinator that PR is ready for merge via comment mention
Demonstrate changes to the team for questions and comments.
(Note: Only required for issues of size
Medium
or larger)✅ Post-merge checklist