`getBooleanInput` ignores `options.required`
Describe the bug
- Calling
getBooleanInputto read an unset input - Calling
getBooleanInputwith{ required: false }to read an unset input throws an error
Expected behavior
getBooleanInput should function the same as getInput in the following ways:
- It respects
options.required - Inputs are not required by default
For future visitors, this issue was discussed when the getBooleanInput function was added and it was decided against (signed off by @thboop):
the function is used to deal with boolean input, It should have a default value in action.yml if it is not necessary
ref: https://github.com/actions/toolkit/pull/725#issuecomment-828120911
However, imo since this function does accept a flag required, it is very surprising that this is not respected. Defaulting to false seems like a sensible default to me.
I'd be happy to contribute a PR for this if you agree @thboop
In the getBooleanInput function, it uses getInput to get the value from action. The required: false will be dealt with the getInput and return the default value. 👀
Could you provide a simple reproducing repository?
actions/setup-dotnet@v2 used to work without passing include-prerelease to it until yesterday...
Today, I get this:
Run actions/setup-dotnet@v2
with:
dotnet-version: 3.1.x
env:
DOTNET_NOLOGO: true
NODE_OPTIONS: --max-old-space-size=4096
Error: Input does not meet YAML 1.2 "Core Schema" specification: include-prerelease
Support boolean input list: `true | True | TRUE | false | False | FALSE`
This appears to have been broken recently (although it's not clear to me how actions/setup-dotnet updates their dependency on actions/tooklit).
According to the code, the actions/setup-dotnet@v2 action actually has a default of false since 2 months ago (so setting a default does not save you here): https://github.com/actions/setup-dotnet/blob/a351d9ea84bc76ec7508debf02a39d88f8b6c0c0/action.yml#L21
Update: I just realized my actions/setup-dotnet had the following clause:
include-prerelease: ${{ matrix.dotnet-prerelease }}
While there was no setting for dotnet-prerelease in the matrix, resulting in the value being explicitly undefined. This might be the cause of the problem I'm observing (the behavior nevertheless changed very recently, but I'm willing to consider it is my fault and this only accidentally worked before).
This is also problematic when trying to run tests for your action, as you have to manually set inputs via envrionment.
I've defined a default in action.yaml, but it has no impact when running tests.
That has nothing to do with the issue here which is about "required" being ignored, does it?
That has nothing to do with the issue here which is about "required" being ignored, does it?
I think it does actually. The argument above was saying you could provide a default in the YAML for the action, but you can't do that when the YAML isn't being used.
I was providing a different use case where the workaround isn't valid.
I see this issue is now pretty old and no one is addressing it - this is a particularly annoying one for us as we are using two approaches to check our action changes aren't going to break CIs and neither picked up on this because:
- https://github.com/rhysd/actionlint correctly doesn't mind an optional boolean parameter without a default
- unit tests mocked out the getBooleanInput so masked the problem
So now we're going to write another "validator" because this hasn't been fixed to ensure that all boolean inputs have a default value if not required.
I'd like to push this once more. Why do I need to mock calls to getBooleanInput in every test to set the default value? This is highly counterintuitive, as evidenced by people arriving here and the linked issues.
I see three possible solutions:
- Introduce a hard dependency on
action.ymlto read the default value (which seems undesirable), - return
falseif the variable is not set in the environment and marked as not required, - add a
strictflag to the options which lets users opt-in to trying YAML-boolean parsing first, truthiness-testing second, which in combination withrequired: falsewould achieve the same outcome.
Right now, this is just another nuisance.