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

Checking for presence of Chocolatey license file in tab expansion PowerShell script generates $error #3218

Closed
2 tasks done
aetonsi opened this issue Jun 19, 2023 · 5 comments · Fixed by #3221
Closed
2 tasks done
Assignees
Milestone

Comments

@aetonsi
Copy link

aetonsi commented Jun 19, 2023

Checklist

  • I have verified this is the correct repository for opening this issue.
  • I have verified no other issues exist related to my request.

Is Your Feature Request Related To A Problem? Please describe.

Hi, i'd like to suggest a simple micro improvement.
The following lines in the "Tab Expansion" helper check for the presence of a license file:

try {
$licenseFile = Get-Item -Path "$env:ChocolateyInstall\license\chocolatey.license.xml" -ErrorAction Stop
if ($licenseFile) {
# Add pro-only commands
$script:chocoCommands = @(

But it does so by relying on a try{}catched Get-Item -ErrorAction Stop. This doesn't print any error, but it still populates the automatic variable $Error with an unneeded exception.
I am importing chocolateyProfile.psm1 which in turn imports this file, in my powershell's $PROFILE file.
At the end of my profile file i do a simple error checking on $Error. Ever since chocolatey v2 came out, i always have this false positive:

> $error

Get-Item: C:\ProgramData\chocolatey\helpers\ChocolateyTabExpansion.ps1:66:20
Line |
  66 |  … censeFile = Get-Item -Path "$env:ChocolateyInstall\license\chocolatey …
     |                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | Cannot find path 'C:\ProgramData\chocolatey\license\chocolatey.license.xml' because it does not exist.

This also causes the $Error variable to always start off with 1 item in every single PS session.

Describe The Solution. Why is it needed?

A very simple solution would be to use Test-Path instead of Get-Item, and then use get-item to fetch the file if needed.

Additional Context

Related Issues

(maybe) #2851

@corbob
Copy link
Member

corbob commented Jun 19, 2023

Well this is an interesting one... I am seeing this on a VM, but it looks like there should be Pester tests testing this (at least within PowerShell 2... As well as this code:

catch {
# Remove the error that last occurred from $error so it doesn't cause any
# issues for users, as we're deliberately ignoring it.
if ($error.Count -gt 0) {
$error.RemoveAt(0)
}
}

Which should be removing the error (and when I manually run those, the error is removed).

@corbob corbob self-assigned this Jun 20, 2023
corbob added a commit to corbob/choco that referenced this issue Jun 20, 2023
The method we were using to clear the error variable wasn't clearing the
variable. Instead of trying this way, it's better to use Test-Path to
verify if a file exists.
@gep13 gep13 added this to the 2.1.0 milestone Jun 20, 2023
corbob added a commit to corbob/choco that referenced this issue Jun 20, 2023
corbob added a commit to corbob/choco that referenced this issue Jun 20, 2023
The method we were using to clear the error variable wasn't clearing the variable. Instead of trying this way, it's better to use Test-Path to verify if a file exists.
vexx32 added a commit that referenced this issue Jun 20, 2023
(#3218) Update Tab Expansion to use Test-Path
@gep13 gep13 added 4 - Done and removed 3 - Review labels Jun 20, 2023
@gep13 gep13 closed this as completed Jun 20, 2023
@aetonsi
Copy link
Author

aetonsi commented Jun 21, 2023

Well i don't know why the $error.removeat(0) doesn't work, the error just spills into the powershell session.
Maybe that $error references a local $error (like $local:error), and so my global $error is not affected by .removeat(0)? Just speculating..
Anyway, thanks for the fix, will this be integrated in a minor or in a patch version?

@corbob
Copy link
Member

corbob commented Jun 21, 2023

Anyway, thanks for the fix, will this be integrated in a minor or in a patch version?

@aetonsi the fix is assigned to the 2.1.0 milestone, so it should come out with that release when it is ready. At this point I can't say for sure when that might be, but I do expect it to be the next release.

In the meantime, a workaround you could consider is moving the test for errors to before the Chocolatey profile loading, and thus circumvent this issue.

@aetonsi
Copy link
Author

aetonsi commented Jun 26, 2023

Thank you again, have a nice day.

@gep13 gep13 changed the title Check license presence without generating $errors in helper file(s) Check for presence of Chocolatey licensed file without generating $errors in helper file(s) in tab expansion PowerShell script Jun 27, 2023
@gep13 gep13 changed the title Check for presence of Chocolatey licensed file without generating $errors in helper file(s) in tab expansion PowerShell script Checking for presence of Chocolatey license file in tab expansion PowerShell script generates $error Jun 27, 2023
gep13 added a commit that referenced this issue Jun 29, 2023
* release/2.1.0: (52 commits)
  (maint) If statement formatting change
  (#3224) Add Assembly Loaded configuration option
  (maint) Resave without BOM
  (#3224) Update version check tests
  (#3174) Store non-normalized package version
  (maint) Add required whitespace
  (#3225) Add Pester Tests to ensure environment
  (#3201 #3225) Re-instate setting of config values
  (maint) Remove env variable for release version
  (maint) Fix encoding of file
  (#3194) Add tab completion for cache command
  (#2854) Re-save file with UTF-8 with BOM
  (#3218) Update Tab Expansion to use Test-Path
  (#3218) Remove try catch block for tab completion
  (build) Use latest Chocolatey.Cake.Recipe package
  (#2854) Do Write-Error instead of Write-Warning
  (#2854) Pester tests to Get-ChocolateyConfigValue
  (maint) Resave Get-ChocolateyConfigValue as CRLF
  (#2854) Add helper to read config values
  (#3214) Add Pester tests to ensure cache cleared
  ...
@choco-bot
Copy link

🎉 This issue has been resolved in version 2.1.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

gep13 added a commit that referenced this issue Jun 29, 2023
* master:
  (maint) If statement formatting change
  (#3224) Add Assembly Loaded configuration option
  (maint) Resave without BOM
  (#3224) Update version check tests
  (#3174) Store non-normalized package version
  (maint) Add required whitespace
  (#3225) Add Pester Tests to ensure environment
  (#3201 #3225) Re-instate setting of config values
  (maint) Remove env variable for release version
  (maint) Fix encoding of file
  (#3194) Add tab completion for cache command
  (#2854) Re-save file with UTF-8 with BOM
  (#3218) Update Tab Expansion to use Test-Path
  (#3218) Remove try catch block for tab completion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants