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

Can't use variable when schema define something as boolean #125

Closed
RomainMou opened this issue Jun 11, 2021 · 4 comments
Closed

Can't use variable when schema define something as boolean #125

RomainMou opened this issue Jun 11, 2021 · 4 comments
Labels
bug Something isn't working

Comments

@RomainMou
Copy link

RomainMou commented Jun 11, 2021

Describe the bug
When something is define as boolean in the schema, like ignore_errors when the value is set by a variable, vscode show an error telling Incorrect type. Expected "boolean".yaml-schema: BeremothTaskModel, even with the filter bool to force the type of the variable.

To Reproduce
Create a task like:

- name: "Demo error"
  fail:
    msg: fail except in check mode
  ignore_errors: "{{ ansible_check_mode | bool }}"

Expected behavior
No error about the type, at least when the type is forced (if | bool is specified this trigger a warning in the default configuration of ansible-lint, not without, so it'll be better if this isn't needed).

Platform:
Be sure vscode and extension are the latest versions and report here the
operating system you used. If you used remote-ssh or WSL. Please check if the
bug can be reproduced on Linux or MacOS.

  • OS: Archlinux
@RomainMou RomainMou added the bug Something isn't working label Jun 11, 2021
@RomainMou
Copy link
Author

Another problem link to that kind of definition:
When this is use in a tasks, pre_tasks or post_tasks of a playbook, like:

- hosts: all
  gather_facts: False
  pre_tasks:
    - name: test
      fail:
        msg: test
      ignore_errors: "{{ ansible_check_mode | bool }}"
  roles:
    - test

This show two errors:

  • the type of ignore_errors, like said previously,
  • one on the task level: Missing property "action".yaml-schema: BeremothTaskModel. The error is not present when ignore_errors is define by true or false.

@ssbarnea
Copy link
Member

Indeed that is a real issue because if we enable use of str in addition to bool it will fail to identify real bugs like plain strings.

My guess is that we should create a JinjaStringModel which would only allow strings that are jinja2 constructs, basically requiring something like {.*} as jinja could have {#, {% and {{.

Does anyone have a better idea?

@webknjaz webknjaz added the new label Aug 16, 2021
@lilatomic
Copy link

I can't reproduce this working off of source ( ansible-language-server 71d759d, vscode-ansible 5ef7df7 ) or from the latest from the marketplace.

I tried looking through the schema definitions, but I didn't find where we forced this to be a boolean. I thought it was possible that this error was coming from the YAML vscode extension, but that didn't reproduce it either.


For resolving the underlying desired result (analysing the type of the Jjinja return and validating that), ansible+jinja is too powerful for simple schema verification to properly analyse. Adding a JinjaStringModel IMO doesn't help that much with the analysis, as it is a subtype of all types. That is, ignore_errors needs to accept bool | JinjaStringModel, and retries needs to accept int | JinjaStringModel, so a JinjaStringModel can be used almost anywhere, and doesn't really convey type information. Worse, it's a "valid" value regardless of the type that the JinjaStringModel actually returns. We could have a Generic form of the type (something like int | JinjaExpr<int>). But now we have the problem of knowing the return type of a Jinja expression. And if we already have that, we don't really need yaml schema validation; we can just raise that validation from whatever is determining the type of the Jinja expressions.

It turns out that It's not too hard to inspect a Jinja AST:

>>> jinja2.Environment().parse("{{ true }}")
Template(body=[Output(nodes=[Const(value=True)])])
>>> jinja2.Environment().parse("{{ ansible_check_mode }}")
Template(body=[Output(nodes=[Name(name='ansible_check_mode', ctx='load')])])

We could then look up the symbol in a table of symbols which would be defined at that scope. We might be able to share some of that code with Ansible itself. Although I suspect some refactoring would be necessary, as we're interested more in the provenance and type of variables than their values. One easy (ish) trick to do type validations is to traverse the program, but instead of keeping track of values in assignments we keep track of their types. For example, if we had set_fact: { "x": 5 } instead of us storing {"x": 5}, we store {"x": int}.

@ssbarnea
Copy link
Member

Closing this because the codebase changed completely since the bug was reported. We no longer use YAML/JSON schemas to validate these files.

@ssbarnea ssbarnea removed the new label Sep 27, 2021
ssbarnea added a commit that referenced this issue Mar 15, 2024
Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants