GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix #1417 modulehelp false positives by Jawz84 · Pull Request #1418 · PowerShell/PSScriptAnalyzer

Conversation

@Jawz84

@Jawz84 Jos Koelewijn (Jawz84) commented Feb 17, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

PR Summary

Fixes #1417

When looking up module version, if the module is not installed, also check if it is loaded and get module version from loaded module
Add 'AttachAndDebug' to paramBlackList, so it does not throw false positives for DEBUG builds.
Small bugfix for the actual blacklist check, the property 'name' of the $parameter variable needs to be used to check against the blacklist.

I have tested this by running:

# in pwsh.exe
.\PSScriptAnalyzer\build.ps1 -clean
.\PSScriptAnalyzer\build.ps1
.\PSScriptAnalyzer\build.ps1 -test
# no errors
# in powershell.exe
.\PSScriptAnalyzer\build.ps1 -clean
.\PSScriptAnalyzer\build.ps1
.\PSScriptAnalyzer\build.ps1 -test
# no errors

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Change is not breaking
  • Make sure all .cs, .ps1 and .psm1 files have the correct copyright header
  • Make sure you've added a new test if existing tests do not effectively test the code changed and/or updated documentation
  • This PR is ready to merge and is not Work in Progress.
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Jos Koelewijn Jos Koelewijn
fix modulehelp false positives
b70e444 
When looking up module version, if the module is not installed, also check if it is loaded and get module version from loaded module
Add 'AttachAndDebug' to paramBlackList, so it does not throw false positives for DEBUG builds.
Small bugfix for the actual blacklist check, the property 'name' of the $parameter variable needs to be used to check against the blacklist.
@bergmeister

Christoph Bergmeister (bergmeister) commented Feb 17, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

I've re-run that failed job on Ubuntu 18.04 but it failed again with that 1 failure. I've re-run CI on master and had the same result. Sometimes it is upstream image changes that can cause that, therefore this 1 failures is not related to your change,
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Jawz84

Jos Koelewijn (Jawz84) commented Feb 17, 2020

Copy link
Copy Markdown
Contributor Author

Oh! I haven't tested on Linux, just pwsh on Windows. Could look into that later
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bergmeister

Christoph Bergmeister (bergmeister) commented Feb 17, 2020

Copy link
Copy Markdown
Collaborator

This failure started to happen in master on that specific image as well, therefore not something related to your change, so don't worry
Jawz84 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Jawz84

Jos Koelewijn (Jawz84) commented Feb 17, 2020

Copy link
Copy Markdown
Contributor Author

I just realized I made one unnecessary change. Because of the trick with temporarily setting $env:psmodulepath, the module will always appear 'installed'. I'll revert that part.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Jos Koelewijn Jos Koelewijn
undo check for loaded module, because $env:psmodulepath is set to .\o…
938696f 
…ut\ module during testing, the built module will always appear 'installed'.
bergmeister
Christoph Bergmeister (bergmeister) reviewed Feb 17, 2020
View reviewed changes
Comment thread Tests/Engine/ModuleHelp.Tests.ps1 Outdated
$commands = Get-Command -FullyQualifiedModule $ms -CommandType Cmdlet,Function,Workflow # Not alias
# Because on Windows Powershell we have workflow, we need to include it there, but in pwsh, we can't. This makes sure this works on both.
$commandTypes = ([System.Enum]::GetNames("System.Management.Automation.CommandTypes") -match "^Cmdlet$|^Function$|^Workflow$")
$commands = Get-Command -FullyQualifiedModule $ms -CommandType $commandTypes

@bergmeister Christoph Bergmeister (bergmeister) Feb 17, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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.

$ms is just PSScriptAnalyzer in this case if I read correctly and PSSA only exports 3 cmdlets, no function/alias/workflow at all, therefore why not simplify it to just:

$commands = Get-Command -FullyQualifiedModule $ms

I could reproduce the error where it complains about workflows locally and just using the proposed line worked fine as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@Jawz84 Jos Koelewijn (Jawz84) Feb 20, 2020

Copy link
Copy Markdown
Contributor Author

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, makes a lot of sense. will do that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
bergmeister
Christoph Bergmeister (bergmeister) reviewed Feb 17, 2020
View reviewed changes
Comment thread Tests/Engine/ModuleHelp.Tests.ps1
$commands =$null
$paramBlackList = @()
$paramBlackList = @(
'AttachAndDebug' # Reason: When building with DEGUG configuration, an additional parameter 'AttachAndDebug' will be added to Invoke-ScriptAnalyzer and Invoke-Formatter, but there is no Help for those, as they are not intended for production usage.

@bergmeister Christoph Bergmeister (bergmeister) Feb 17, 2020

Copy link
Copy Markdown
Collaborator

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 change absolutely makes sense, I could reproduce the failure locally using a debug build. Thanks for taking the time to fix it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Jawz84 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
bergmeister
Christoph Bergmeister (bergmeister) approved these changes Feb 17, 2020
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment

Copy link
Copy Markdown
Collaborator

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.

Thanks for taking the time. Good improvements! It's good to have everything work 'out of the box', no matter in which configuration it was built.
Just one small suggestion to minimize/simplify one change but otherwise looks good.

On an unrelated note: I saw your initial change to some of the Get-Module version logic and actually think most of it is not necessary and not even right in all cases and should rather be replaced by something like (Get-Command Invoke-ScriptAnalyzer).Module.Version because only this was will it load the right module into memory and return the correct version. But let's defer that to another PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Jos Koelewijn Jos Koelewijn
simplify command discovery
58fb6c6
@Jawz84

Jos Koelewijn (Jawz84) commented Feb 20, 2020

Copy link
Copy Markdown
Contributor Author

Could you have another look Christoph Bergmeister (@bergmeister) ?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
bergmeister
Christoph Bergmeister (bergmeister) approved these changes Feb 21, 2020
View reviewed changes

@bergmeister Christoph Bergmeister (bergmeister) left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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.

Awesome, thanks, fully happy now :-)
Rob Holt (@rjmholt) There is new test failure in master (which is the one we see here) and it happens only on Ubuntu18 and it is another one of your compatibility tests, shall we disable it on Linux as well since we know that those sporadic failures in your compatibility tests come and go depending on image Ubuntu image updates?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Jawz84 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bergmeister
Christoph Bergmeister (bergmeister) requested a review from Rob Holt (rjmholt) February 21, 2020 08:08
@bergmeister
Christoph Bergmeister (bergmeister) merged commit 46e89de into PowerShell:master Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bergmeister Christoph Bergmeister (bergmeister) bergmeister approved these changes
@rjmholt Rob Holt (rjmholt) Awaiting requested review from rjmholt

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ModuleHelp.Tests.ps1 throws false positives

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@Jawz84 @bergmeister
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.