toolkit icon indicating copy to clipboard operation
toolkit copied to clipboard

`getBooleanInput` ignores `options.required`

Open RA80533 opened this issue 4 years ago • 7 comments

Describe the bug

  1. Calling getBooleanInput to read an unset input
  2. Calling getBooleanInput with { 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

RA80533 avatar Jun 14 '21 12:06 RA80533

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

rethab avatar Mar 04 '22 13:03 rethab

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?

yi-Xu-0100 avatar Sep 20 '22 02:09 yi-Xu-0100

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).

RomainMuller avatar Sep 27 '22 08:09 RomainMuller

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.

Relequestual avatar Jul 07 '23 14:07 Relequestual

That has nothing to do with the issue here which is about "required" being ignored, does it?

Vampire avatar Jul 07 '23 15:07 Vampire

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.

Relequestual avatar Jul 07 '23 16:07 Relequestual

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:

  1. https://github.com/rhysd/actionlint correctly doesn't mind an optional boolean parameter without a default
  2. 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.

LittleColin avatar Sep 20 '24 13:09 LittleColin

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:

  1. Introduce a hard dependency on action.yml to read the default value (which seems undesirable),
  2. return false if the variable is not set in the environment and marked as not required,
  3. add a strict flag to the options which lets users opt-in to trying YAML-boolean parsing first, truthiness-testing second, which in combination with required: false would achieve the same outcome.

Right now, this is just another nuisance.

Radiergummi avatar Jul 13 '25 07:07 Radiergummi