Skip to content

feat: Support checkver's option "THROW_ERROR"#5

Merged
rashil2000 merged 1 commit intoScoopInstaller:mainfrom
CrendKing:main
Apr 14, 2022
Merged

feat: Support checkver's option "THROW_ERROR"#5
rashil2000 merged 1 commit intoScoopInstaller:mainfrom
CrendKing:main

Conversation

@CrendKing
Copy link
Copy Markdown

For issue ScoopInstaller/Scoop#4863.

Other than simply passing along the new option to underlying script, I noticed that the environment variables from GitHub Action yaml's env section are always converted to strings. For example, I tested the following in excavator.yml:

foo: 0
foo: 1
foo: '0'
foo: '1'
foo: true
foo: false
foo: 'true'
foo: 'false'

For all these, [bool] $env.foo are always true and ($env.foo).GetType() are always System.String. So the existing [bool] $env:SKIP_UPDATED is always true. Not very impacting since most people use '1' anyways. But if we want to default the new THROW_ERROR to false, I think it's better to fix this and make it consistent. There are ways to convert string to boolean in PowerShell in more complete way, but I think checking equalization to '1' is probably good enough. Let me know if this needs change.

@rashil2000
Copy link
Copy Markdown
Member

Nice observation!!

@rashil2000 rashil2000 requested a review from issaclin32 April 14, 2022 12:56
@rashil2000 rashil2000 merged commit 24f1008 into ScoopInstaller:main Apr 14, 2022
@rashil2000
Copy link
Copy Markdown
Member

We'll have to revert this PR for now. The changes in Scoop core haven't been released yet, and as a result all the Excavator runs are failing with this error.

image

/cc @issaclin32 @CrendKing we should wait for Scoop core release and then merge again.

@CrendKing
Copy link
Copy Markdown
Author

Sorry. I forgot to mention this. Alternatively, we could change auto-pr to use the splat form to pass param (i.e. Add "ThrowError" to the $param HashTable only if the value is $true). Do you want that? I could do it.

@rashil2000
Copy link
Copy Markdown
Member

Changes to auto-pr.ps1 would again happen in the develop branch, right? We would still have to wait for a release.

@CrendKing
Copy link
Copy Markdown
Author

Of course. If you think this kind of changes happen rarely, and we don't need to bother a future-proof approach, then let's wait.

@rashil2000
Copy link
Copy Markdown
Member

Instead, can you do something like this:

if ($env:SPECIAL_SNOWFLAKES) { $params.Add('SpecialSnowflakes', ($env:SPECIAL_SNOWFLAKES -split ',')) }

@CrendKing
Copy link
Copy Markdown
Author

To auto-pr.ps1?

@rashil2000
Copy link
Copy Markdown
Member

@CrendKing
Copy link
Copy Markdown
Author

#6

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.

3 participants