-
Notifications
You must be signed in to change notification settings - Fork 4
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
add timeout connexion and add check on open connect #2
base: master
Are you sure you want to change the base?
Conversation
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 for making those corrections.
Someone will have to look into the status check on this one. I don't think the CICD has executed for this in a long time. |
Hello, |
Well, I tried to migrate to Pester 5 but I encountered errors during the Help.Test.ps1 tests although it is correct ... If you find the source of the problem, I am interested! |
I got the appveyor hook working again so the PR build should work when you push another commit |
I don't know why but the tests cannot find a description at the parameters although I did not touch anything on this part. Do you have an idea ? |
I'll take a look in the morning at the test issues. I think some PSScriptAnalyzer and Pester updates broke things in a lot of our modules, i'd have to dig deeper into this one tomorrow. Re pipeline indentation: https://devblogs.microsoft.com/powershell/release-of-powershell-script-analyzer-1-18-1/ I don't know if an official standard has been set, but our modules all follow the indent-after-first-continuation style, which is now the default style for all powershell formatters to my knowledge (aside from outdated VSCode settings as mentioned above). |
@@ -6,7 +6,7 @@ $Script:SourceRoot = Join-Path -Path $ModuleRoot -ChildPath $ModuleName | |||
Describe "All commands pass PSScriptAnalyzer rules" -Tag 'Build' { | |||
$rules = "$ModuleRoot\ScriptAnalyzerSettings.psd1" | |||
$scripts = Get-ChildItem -Path $SourceRoot -Include '*.ps1', '*.psm1', '*.psd1' -Recurse | | |||
Where-Object FullName -notmatch 'Classes' | |||
Where-Object FullName -notmatch 'Classes' |
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.
(still not indented)
@@ -1,4 +1,4 @@ | |||
function ImportModule | |||
function Import-BuildModule |
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 wouldn't normally name internal functions with *-*
syntax, but thats personal preference. Makes it confusing as it looks like you're using a cmdlet when you're using a local function instead. You can leave this, but just pointing out why it was named as it was originally.
@@ -1,34 +1,34 @@ | |||
task Pester { | |||
$requiredPercent = $Script:CodeCoveragePercent | |||
Import-Module Pester -Force |
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.
we don't support pester 5+ so I can't really help you here, the help tests rely on pester 4 functionality, pester 5 appears to be completely throwing out variables from the outer scope within Context blocks. The tests should work fine on Pester 4.10 (they are still passing for me on https://github.com/loanDepot/SnowSQL master branch. I can't test 4.10 on your branch since your changes here will not run on 4.10 due to the changes below being 5.0 functionality).
I would recommend rolling the pester changes back, honestly, We've had nothing but trouble with pester 5 historically. Feel free to replace with a forced load of
Import-Module Pester -RequiredVersion 4.10.1 -Force
and a silent install of 4.10.1 if needed on appveyor (not sure how that works, really).
PSScriptAnalyzer and Pester updates are pretty involved sometimes, and we normally bulk update all our modules at once to a new version of pester once we know what we're getting ourselves into rather than piecemealing like this. If you can figure out how to make Pester 5 work here, be my guest, but it looks like the $node variables are completely gone once you enter the Context block in Pester 5, so you'd have to work around that somehow. I'm sure there's a way if you want to figure it out, but since we upgrade Pester in bulk across 50+ modules by merging a common base module repo in, we'd rather just solve it there once and leave this working as-is on Pester 4.10.1 until then, and merge that in one big batch to all our modules as we normally do.
See comment on pester block. Pester 5 breaks things, I'd roll back and just force an import / install of 4.10.1. Pester 5 is a large undertaking and has major, massive breaking changes to the way it functions. See https://github.com/pester/Pester/releases#actual-breaking-changes You may be able to fix help.tests by shifting the outer logic into a beforeall block and refactoring the context block to take testcases instead of coming from a loop. But as pointed out in the comments, i'd rather just solve that once in the future and merge to all our modules instead of having a merge conflict on this one, so rolling back to pester 4.10.1 is easiest on us (and probably you as well) |
Hello,
I got your project to do actions on Snowflake. Not knowing Snowflake very well, I encountered some problems connecting to it so I will suggest my modifications.
Good reading