Skip to content
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

Boolean inputs are not actually booleans in composite actions #2238

Open
flobernd opened this issue Nov 2, 2022 · 14 comments
Open

Boolean inputs are not actually booleans in composite actions #2238

flobernd opened this issue Nov 2, 2022 · 14 comments
Assignees
Labels
bug Something isn't working keep Label can be added as soon as we are sure the work on the issue is necessary triaged Needs extra internal investigation before adding ready-for-dev

Comments

@flobernd
Copy link

flobernd commented Nov 2, 2022

Describe the bug

I'm not sure if closed issues are monitored, as there was no reaction from official side at all. This is a new issue related to #1483.

For composite actions boolean inputs are not actually booleans.

To Reproduce

inputs:
  ...
  generate-release-notes:
    description: ...
    required: false
    type: boolean
    default: false

runs:
  using: composite
  steps:
    - name: Create Release
      uses: actions/github-script@v6
      with:
        script: |
          github.rest.repos.createRelease({
            owner: context.repo.owner,
            repo: context.repo.repo,
            ...
            generate_release_notes: ${{ inputs.generate-release-notes && 'true' || 'false' }}
          });

Caller:

- uses: flobernd/actions/github/create-release@master
  with:
    tag-name: v1.2.4
    generate-release-notes: true

This line always evaluates to false:

generate_release_notes: ${{ inputs.generate-release-notes && 'true' || 'false' }}

The explicit syntax does incorrectly evaluate to false as well:

generate_release_notes: ${{ inputs.generate-release-notes == true && 'true' || 'false' }}

Correct behavior is only observed when using string semantics:

generate_release_notes: ${{ inputs.generate-release-notes == 'true' && 'true' || 'false' }}

Expected behavior

generate_release_notes: ${{ inputs.generate-release-notes && 'true' || 'false' }}
generate_release_notes: ${{ inputs.generate-release-notes == true && 'true' || 'false' }}

Evaluates to true.

Runner Version and Platform

GitHub managed runners (latest version). All platforms.

@flobernd flobernd added the bug Something isn't working label Nov 2, 2022
@ChristopherHX
Copy link
Contributor

Would be nice to have non string inputs in composite actions, especially the object type would be great in my opinion

inputs:
  ...
  myobjinput:
    description: ...
    required: false
    type: object

runs:
  using: composite
  steps:
  - run: |
      echo '${{ tojson(inputs.myobjinput) }}'

Fun fact this composite action will also use the type string for myobjinput, because the type property is not parsed / supported in composite actions yet. Just the parser is instructed to be relaxed to not throw an error if you invent your own properties, I think you can add anything under an input yaml mapping.

Action Inputs are still saved in a Dictionary<string, string>, which cannot even store a true boolean, I'm not affiliated with GitHub, only a contributor of one bug fix.

@ssallmen-pro
Copy link

I bumped on this also today. I am calling my composite action from a workflow with a boolean input as follows:

...
on:
  workflow_dispatch:
    inputs:
      realRun:
        description: Really run?
        default: false
        type: boolean
...
jobs:
  run-tests:
      - uses: my-org/test-run@main
        with:
          realRun: ${{ inputs.realRun == true }}

And this is how my composite action my-org/test-run@main looks like from some relevant parts (some debugging added and a lot of things omitted here):

...
inputs:
  realRun:
    description: Really run?
    default: false
    type: boolean
...
runs:
  using: composite
  steps:
    - name: Check realRun input value
      shell: bash
      run: echo "realRun==${{ inputs.realRun == true }}"

    - name: Run tests
      if: ${{ inputs.realRun == true }}
      shell: bash
      run: ${{ github.action_path }}/bin/run_tests.sh

That will always echo realRun==false on the first step and will never run the second step, independent on whether the input value is given as boolean true or false. That is because it seems the realRun input value is treated as string in the composite action.
I know I can get it working by using string 'true' in the expressions above, instead of boolean true, but it will work only as long as this bug with treating boolean inputs as strings in a composite action has been fixed.

@veleek
Copy link

veleek commented Nov 17, 2022

I was hoping to do something similiar with the output of a composite action, but there doesn't seem to be any way to force it to be a boolean either. I'm using:

outputs:
  image_exists:
    description: true if an image with the given exists, false otherwise
    value: ${{ fromJSON(steps.verify_image_exists.outputs.image_exists) }}

This shouldn't be surprising given that it is documented but it's still frustrating and you end up with the same "always evaluates to false" problem.

@AvaStancu AvaStancu added keep Label can be added as soon as we are sure the work on the issue is necessary triaged Needs extra internal investigation before adding ready-for-dev labels Nov 23, 2022
@machulav
Copy link

machulav commented Jan 18, 2023

Agree with @veleek - this is very frustrating to have conditions like the following:

if: needs.get-pr-labels.outputs.build-label-present == 'true' || needs.get-pr-labels.outputs.deploy-label-present == 'true'

It would be nicer if the outputs were boolean, so the condition looks like this:

if: needs.get-pr-labels.outputs.build-label-present || needs.get-pr-labels.outputs.deploy-label-present

The outputs in the composite actions look like this:

on:
  workflow_call:
    outputs:
      build-label-present:
        description: "cicd:build label is present in the PR"
        value: ${{ contains(github.event.pull_request.labels.*.name, 'cicd:build') }}
      deploy-label-present:
        description: "cicd:deploy:* label is present in the PR"
        value: ${{ contains(toJSON(github.event.pull_request.labels.*.name), 'cicd:deploy:') }}

@sremp
Copy link

sremp commented Jan 20, 2023

+1 to this. Just recently ran into this exact issue, with the only reasonable workaround being to use string semantics as detailed in the original post.

@justinmchase
Copy link

This appears to be true for docker based actions as well.

@SatishReddyBethi
Copy link

+1

@agileshaw
Copy link

Just ran into this issue as well. There doesn't seem to be any related information on the official documentation, and it is very non-intuitive to have inputs support multiple types in workflows and "normal" actions but not in composite actions. I hope this behavior can be changed.

@wiegell
Copy link

wiegell commented May 15, 2023

I just spend several hours trying to pass a true value... The way to do it is:

Job:


env:
  SOME_ENV: ${{ github.event_name == 'pull_request' }}

jobs:
  somejob:
    name: Build
    runs-on: ubuntu-latest

    steps:
      - name: Set up build
        uses: ./.github/actions/xxx
        with:
          some-input-value: ${{ env.SOME_ENV == 'true' }}

Composite:

name: Set up build
description: Steps before build

inputs:
  some-input-value:
    description: "Holy moly this was difficult"
    required: false
    default: false
    type: boolean

runs:
  using: composite

steps:
    - name: "Input flag:"
      run: echo "Inputflag: " && echo ${{ inputs.some-input-value== 'true'  }}
      shell: bash

    - name: "Some action"
      uses: someorg/someaction@v999
      if: ${{ inputs.some-input-value == 'true' }}

This makes no sense at all and is not documented.

devholic added a commit to devholic/nix-quick-install-action that referenced this issue Aug 1, 2023
thecristen referenced this issue in mbta/workflows Aug 7, 2023
This avoids failures when running on PRs from forks.

We do it in this convoluted way because you can't access secrets
directly from `if` blocks: actions/runner#520
AsturaPhoenix added a commit to AsturaPhoenix/trip_planner_aquamarine that referenced this issue Aug 12, 2023
Writes to a cache from a pull request are not visible elsewhere, so
* Build the caches in postsubmit so that they will be available eventually
  * While we're at it, run all tests as a postsubmit in case of rebase issues.
* Don't save caches in the PR so that they won't spam persistence.

https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache

While we're at it, switch to environments for better security since secrets could otherwise be available to pull requests before review, and add the patrol cache.

Fun fact: actions/runner#2238
AsturaPhoenix added a commit to AsturaPhoenix/trip_planner_aquamarine that referenced this issue Aug 12, 2023
Writes to a cache from a pull request are not visible elsewhere, so
* Build the caches in postsubmit so that they will be available eventually
  * While we're at it, run all tests as a postsubmit in case of rebase issues.
* Don't save caches in the PR so that they won't spam persistence.
* Also don't bother generating an emulator image for caching on PR workflows.

https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache

While we're at it, switch to environments for better security since secrets could otherwise be available to pull requests before review, and add the patrol cache.

Fun fact: actions/runner#2238
AsturaPhoenix added a commit to AsturaPhoenix/trip_planner_aquamarine that referenced this issue Aug 12, 2023
Writes to a cache from a pull request are not visible elsewhere, so
* Build the caches in postsubmit so that they will be available eventually
  * While we're at it, run all tests as a postsubmit in case of rebase issues.
* Don't save caches in the PR so that they won't spam persistence.
* Also don't bother generating an emulator image for caching on PR workflows.

https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache

While we're at it, switch to environments for better security since secrets could otherwise be available to pull requests before review, and add the patrol cache.

Fun fact: actions/runner#2238
AsturaPhoenix added a commit to AsturaPhoenix/trip_planner_aquamarine that referenced this issue Aug 12, 2023
Writes to a cache from a pull request are not visible elsewhere, so
* Build the caches in postsubmit so that they will be available eventually
  * While we're at it, run all tests as a postsubmit in case of rebase issues.
* Don't save caches in the PR so that they won't spam persistence.
* Also don't bother generating an emulator image for caching on PR workflows.

https://docs.github.com/en/actions/using-workflows/caching-dependencies-to-speed-up-workflows#restrictions-for-accessing-a-cache

While we're at it, switch to environments for better security since secrets could otherwise be available to pull requests before review, and add the patrol cache.

Fun fact: actions/runner#2238
@maschwenk
Copy link

maschwenk commented Aug 21, 2023

Also worth nothing that if expressions are always evaluated to strings https://docs.github.com/en/actions/learn-github-actions/expressions#about-expressions

Then it seems unclear how this will ever work for passing variables from workflow call/dispatch. E.g.

workflow dispatch with boolean 
  -> calls composite action
      with:
        variable: ${{ inputs.from_workflow_dispatch_boolean }}   <--- this could start as a boolean from the dispatch but be coerced to a string by ${{

iwahbe added a commit to pulumi/pulumi-upgrade-provider-action that referenced this issue Oct 9, 2023
We are seeing provider's merge without review or release. I believe it
is because
`automerge` is set to `"false"`, which is truthy.

GitHub Actions don't mention a `type` field for composite action's
inputs, which makes me
afraid this is just a string. This is substantiated by
actions/runner#2238.
@grossag
Copy link

grossag commented Dec 28, 2023

I am also running into this and trying to sort out how to work around this.

I would suggest not doing a workaround that involves using type: boolean but then treating it as a string, such as if: ${{ inputs.some-input-value == 'true' }}. My reasoning is that if GitHub actually fixes this issue, those workarounds would no longer be expected to function. After a fix to this issue, comparing a boolean == 'true' would return false, which is not what you want in the long-term.

@grossag
Copy link

grossag commented Jan 10, 2024

I have been thinking about this more lately and I'm starting to think that this is not a bug and instead is us using yaml syntax that is not actually supported or documented by GitHub.

workflow_call and workflow_dispatch have inputs sections documented that include the type field, so it led me and others to think that composite actions can have that field too.

However, the inputs documentation for composite actions doesn't document the type field at all. So when we put type: boolean in there, we are just adding a field that GitHub doesn't know about just like if I was to add someflagijustmadeup: true, so it is ignored.

@veleek
Copy link

veleek commented Jan 10, 2024

@grossag, while I think that you're correct in that this is simply not a supported feature of GHActions, I think the rest of this thread makes it very clear that people would REALLY like if it we're supported. The fact that it's nearly identical to other mechanisms for declaring inputs except that it's missing type is confusing on it's own and probably worth doing something about.

sgametrio added a commit to tx-pts-dai/github-workflows that referenced this issue Jan 11, 2024
sgametrio added a commit to tx-pts-dai/github-workflows that referenced this issue Jan 11, 2024
sgametrio added a commit to tx-pts-dai/github-workflows that referenced this issue Jan 11, 2024
Jardo-51 added a commit to maven-flow/prevent-artifact-overwrites that referenced this issue May 6, 2024
Jardo-51 added a commit to maven-flow/prevent-artifact-overwrites that referenced this issue May 6, 2024
Jardo-51 added a commit to maven-flow/prevent-artifact-overwrites that referenced this issue May 6, 2024
Jardo-51 added a commit to maven-flow/prevent-artifact-overwrites that referenced this issue May 6, 2024
@Jardo-51
Copy link

Jardo-51 commented May 6, 2024

This bug just cost me half of a work day. Why isn't anyone addressing this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working keep Label can be added as soon as we are sure the work on the issue is necessary triaged Needs extra internal investigation before adding ready-for-dev
Projects
None yet
Development

No branches or pull requests