Skip to content

Add -EnableExit switch to Invoke-ScriptAnalyzer for exit and return exit code for CI purposes #842

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

Merged
merged 9 commits into from
Jan 21, 2018

Conversation

bergmeister
Copy link
Collaborator

@bergmeister bergmeister commented Dec 9, 2017

To close #840
Behaviour is similar to the equivalent switch in Invoke-Pester, i.e. the shell exits with an exit code equal to the number of error records.

@bergmeister bergmeister changed the title Add -EnableExit switch Work in progress: Add -EnableExit switch Dec 9, 2017
@bergmeister bergmeister changed the title Work in progress: Add -EnableExit switch Add -EnableExit switch Dec 9, 2017
@bergmeister bergmeister changed the title Add -EnableExit switch Add -EnableExit switch to Invoke-ScriptAnalyzer for exit and return exit code CI purposes Dec 9, 2017
@bergmeister bergmeister changed the title Add -EnableExit switch to Invoke-ScriptAnalyzer for exit and return exit code CI purposes Add -EnableExit switch to Invoke-ScriptAnalyzer for exit and return exit code for CI purposes Dec 10, 2017
Copy link
Contributor

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still coming up to speed on the code base, but shouldn't there be an additional test for this new behavior?

@bergmeister
Copy link
Collaborator Author

bergmeister commented Jan 20, 2018

@JamesWTruher Thanks for reminding (and motivating) me to write an automated test. At first I thought this might be difficult due to the cmdlet exiting the shell but at the end it was quite easy due to PowerShell being so awesome.
I have one piece of knowledge to share with you that I learned in one of my last PRs from Kapil: All tests are run twice: Once using .Net Core and the 2nd time with a special configuration mode where the main PSSA functions are being partially mocked (I think to test the functionality of PSSA being used as a .net library)

It "Returns exit code equivalent to number of warnings" {
$process = Start-Process powershell -ArgumentList '-WindowStyle Hidden -Command Import-Module PSScriptAnalyzer; Invoke-ScriptAnalyzer -ScriptDefinition gci -EnableExit' -PassThru -Wait
$process.ExitCode | Should Be 1
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you have is fine, but would this also work?

powershell -command 'import-module psscriptanalyzer; invoke-scriptanalyzer -enableexit -scriptdef gci'
$LASTEXITCODE | Should be 1

Eventually, I would like to see these be portable on other platforms, and the -WindowStyle parameter isn't portable. Eventually, we also need to calculate the name of powershell (since it can be powershell or pwsh, but that can be ignored for now)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is possible. I have changed it to that.

Copy link
Contributor

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with the recent change with regard to -EnableExit and -Fix parameters the readme has a merge conflict. Would you address this?

…nalyzer into enableExit

Merge conflict on syntax line, easy to resolve.
# Conflicts:
#	README.md
@bergmeister
Copy link
Collaborator Author

OK. I resolved it. What do you think about the problem that the repo shows the ReadMe of the development branch by default? Could this be confusing for people to see parameters being documented that are not yet in the latest released version of PSSA?

@JamesWTruher JamesWTruher merged commit e973047 into PowerShell:development Jan 21, 2018
@JamesWTruher
Copy link
Contributor

JamesWTruher commented Jan 21, 2018

Do you happen to know why that was chosen? I need to talk to somebody in the office about it. It doesn't seem necessary or even preferred. I'd like to bring it into sync with how we do PSCore, using master as the main branch and labels for releases.

@bergmeister
Copy link
Collaborator Author

bergmeister commented Jan 21, 2018

@JamesWTruher I don't know why it was chosen like that (remember that PSSA originally came from the community and not MS) but I guess the person was inspired by the GitFlow philosophy where the main development happens in a dev branch with the idea being that master is always green and shippable.
Originally, it was developed to help reduce mistakes with branching and I have seen its usage in my previous team at my company but my personal conclusion is that one still has to know what one is doing and it brings other problems and more complexity. I think it only makes sense to do that in a big project where multiple feature teams have to work on a big product like VSTS.
git-workflow-release-cycle-4maintenance
Do you know when you roughly expect to release the next version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add -EnableExit parameter to ease CI usage
2 participants