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.

Sign up for GitHub

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

Jump to bottom

config-map failing with "Expected string, but was null" in v4.2 #911

Closed
mikocot opened this issue Apr 11, 2023 · 9 comments · May be fixed by #912
Closed

config-map failing with "Expected string, but was null" in v4.2 #911

mikocot opened this issue Apr 11, 2023 · 9 comments · May be fixed by #912
Assignees
@simenandre
Labels
area/cicd impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Milestone
0.87

Comments

@mikocot
Copy link
Contributor

mikocot commented Apr 11, 2023 •
edited
Loading

What happened?

Pipeline failures after release of v4.2.

Pipeline code:

 uses: pulumi/actions@v4
[...]
 config-map: | # provide required configuration
   { prNumber: { value: ${{ github.event.pull_request.number }} } }

Error message
Validation failed: { "prNumber": { "value": "Expected string, but was null" } }. Object should match { [_: string]: { value: string; secret?: boolean; } }

The same happens if ${{ github.event.pull_request.number }} is provided, but it's a number, the error is then:

"Expected string, but was number"

Expected Behavior

No breaking changes in minor release.
Changes in interface communicated in release notes and PR descriptions

Steps to reproduce

See code above

Output of pulumi about

not essentiral here

Additional context

The issue can be fixed by wrapping params in double quotes or ensuring it's a string.

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).
@mikocot mikocot added kind/bug Some behavior is incorrect or out of spec needs-triage Needs attention from the triage team labels Apr 11, 2023
@mikocot
Copy link
Contributor Author

mikocot commented Apr 11, 2023

seems to be related to changes in this PR: #813
but as there is no description it's hard to say if it was intended or not.

@mikocot mikocot changed the title config-map failing with"Expected string, but was null" in v4.2 config-map failing with "Expected string, but was null" in v4.2 Apr 11, 2023
@simenandre
Copy link
Contributor

Hey, thanks for reporting this!

It's related to #813; I'll look at a fix very soon!

@simenandre simenandre added the p1 A bug severe enough to be the next item assigned to an engineer label Apr 11, 2023
@simenandre simenandre self-assigned this Apr 11, 2023
@mikocot
Copy link
Contributor Author

mikocot commented Apr 11, 2023

I can imagine this was by design, but at least the previous version was more forgiving and many are likely to depend on it.
Accepting numbers / bools feels reasonable, but if they are going to be converted to strings anyway, it might be better to be explicit, so I'm not exactly opposed to that change, rather the way it was done :)

This was referenced Apr 11, 2023
Open
Open
@simenandre
Copy link
Contributor

I can imagine this was by design, but at least the previous version was more forgiving and many are likely to depend on it. Accepting numbers / bools feels reasonable, but if they are going to be converted to strings anyway, it might be better to be explicit, so I'm not exactly opposed to that change, rather the way it was done :)

I agree, so I'm pushing for a quick release here. My reasoning was that I naively thought the types defined in Automation APIs setAllConfigMap was what users were adhering too.

@RobbieMcKinstry RobbieMcKinstry added impact/regression Something that used to work, but is now broken area/cicd and removed needs-triage Needs attention from the triage team labels Apr 11, 2023
@RobbieMcKinstry RobbieMcKinstry added this to the 0.87 milestone Apr 11, 2023
@arminrosu
Copy link

For reference, we're seeing the same kinds of errors with boolean types.

In the GHA workflow below, workflow_dispatch (manual trigger) works, but pushes to master fail with:

Error: Input does not meet YAML 1.2 "Core Schema" specification: refresh
Support boolean input list: `true | True | TRUE | false | False | FALSE`

name: Pulumi Update
on:
  push:
    branch:
      - master
  workflow_dispatch:
    inputs:
      refresh:
        description: Should pulumi run refresh?
        default: false
        required: false
        type: boolean
# ...
jobs:
  pulumi-update:
    steps:
      # ...
      - name: Run pulumi up
        uses: pulumi/actions@v4
        id: pulumi-up
        with:
          refresh: ${{ github.event.inputs.refresh }}

Probably github.event.inputs.refresh is undefined, d'uh.

I can fix it, but it's a nuisance considering this was a non-major release.

@mikocot
Copy link
Contributor Author

mikocot commented Apr 11, 2023

I agree, so I'm pushing for a quick release here. My reasoning was that I naively thought the types defined in Automation APIs setAllConfigMap was what users were adhering too.

Haha, it might be a bit far connection for most users :)

@simenandre
Copy link
Contributor

I agree, so I'm pushing for a quick release here. My reasoning was that I naively thought the types defined in Automation APIs setAllConfigMap was what users were adhering too.

Haha, it might be a bit far connection for most users :)

Yeah, I was quite naive 😅

Fix coming up!

@arminrosu
Copy link

Yeah, I was quite naive 😅

I call it "optimistic" 😉. No worries, thanks for the swift response.

@simenandre simenandre added the resolution/fixed This issue was fixed label Apr 12, 2023
@simenandre
Copy link
Contributor

I hope v4.2.1 fixes the issues!

I'm closing this for now, but please let me know if it continues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@simenandre simenandre

Labels
area/cicd impact/regression Something that used to work, but is now broken kind/bug Some behavior is incorrect or out of spec p1 A bug severe enough to be the next item assigned to an engineer resolution/fixed This issue was fixed
Projects
None yet
Milestone
0.87
Development

Successfully merging a pull request may close this issue.

feat: support for config on multiple types on keys and values pulumi/actions
4 participants
@arminrosu @simenandre @RobbieMcKinstry @mikocot