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

false positive truthy failure on travis files #158

Closed
ssbarnea opened this issue Jan 14, 2019 · 12 comments · Fixed by #247
Closed

false positive truthy failure on travis files #158

ssbarnea opened this issue Jan 14, 2019 · 12 comments · Fixed by #247

Comments

@ssbarnea
Copy link
Sponsor Contributor

It seems that yamllint chokes complaining about not using truthy values for travis keys.

Just to be clear: this is about keys not values with name "on".. When used as a key, on have a total different sense than the "on" as a value, which I find acceptable to trigger a warning.

@adrienverge
Copy link
Owner

Hi @ssbarnea,

I confirm the behavior, and it's described in the documentation.

Values like on, yes, etc. can be converted to "true", even if they are keys. For example, run the following code:

echo '
true: 1
on: 2
yes: 3
' | python -c 'import sys, yaml; print(yaml.load(sys.stdin))'

It will produce:

{True: 3}

@ssbarnea
Copy link
Sponsor Contributor Author

Thanks for the update, now we need to find a way to fix this because having it documented does not make the user experience better. So far the only place I found to be conflicting is travis.yaml and to be honest I cannot find any reasom for blaming its syntax here.

Maybe we need an option to look for truthy only for leaf-keys or to disable it for travis files. I am open to suggestions.

Fixing this is not an urgency as I disabled the truthy feature completely for the moment on that small project. Still, we should look for a way to address this in the default tool behaviour, so the user experience will be good and he will not encounter this false-positive.

@adrienverge
Copy link
Owner

having it documented does not make the user experience better.

For people that read the documentation, it does 😉

I'm open to proposals and contributions. Also, feel free to explain what you mean by "travis keys": a clear example of your specific problem would help.

@ssbarnea
Copy link
Sponsor Contributor Author

My mistake to not give an example, https://github.com/pycontribs/tendo/blob/master/.travis.yml#L78

We are lucky the the "on" condition (which is similar to when on ansible) is not very common and is not present in default travis templates. Still, is a core feature which is very old and established so I doubt they will change the file format to pass yamllint-ing.

I don't know what is the right approach to take here but I know how to describe the desired user experience: when user enables yamllint they should not get a false-positive on a travis.yaml file.

@adrienverge
Copy link
Owner

What about a new option for the truthy rule? Something like check-keys: true | false or similar?

@pdericson any thoughts?

@jayvdb
Copy link
Contributor

jayvdb commented Jul 12, 2019

What about a special case for .travis.yml , or having schemas which define key names which could then be whitelisted if they appear in the correct place in the yaml. e.g. use https://github.com/SchemaStore/schemastore/blob/master/src/schemas/json/travis.json for travis

@ghost
Copy link

ghost commented Sep 30, 2019

This issue is not limited to .travis.yml configs. The new GitHub Actions yaml spec has a required on key: https://help.github.com/en/articles/workflow-syntax-for-github-actions#on
E.g.,

name: Pull request based on master
on:
  pull_request:
    branches:
      - master
# ...

yamllint is throwing 2:1 warning truthy value should be one of [false, true] (truthy).

@adrienverge
Copy link
Owner

Hi,

While waiting for someone to contribute a new check-keys option or something equivalent, an option is to disable the truthy rule in yamllint config:

---
extends: default
rules:
  truthy: disable

... or allow on:

---
extends: default
rules:
  truthy:
    allowed-values: ['true', 'false', 'on']

@jameswilliams1
Copy link

jameswilliams1 commented Feb 16, 2020

An easy workaround here, add:

# yamllint disable-line rule:truthy

to any ci lines that use 'on' which avoids having to fully disable truthy (personally I like this rule for Ansible roles and don't want to disable it due to a bug).

Relevant docs page: https://yamllint.readthedocs.io/en/stable/disable_with_comments.html

oalders added a commit to oalders/dot-files that referenced this issue Mar 23, 2020
badouralix added a commit to badouralix/coding-best-practices that referenced this issue Jun 6, 2020
jgstew pushed a commit to jgstew/wait-action that referenced this issue Mar 21, 2021
kerberizer added a commit to ideaconsult/ambit-docker that referenced this issue May 10, 2021
hamishcoleman added a commit to hamishcoleman/n2n that referenced this issue Oct 25, 2021
Logan007 pushed a commit to ntop/n2n that referenced this issue Oct 25, 2021
* Add workaround for 'truthy' warnings in yaml

See adrienverge/yamllint#158
for some more discussion

* Check each yamllint line length warning and clearly markup those which cannot be easily folded into shorter lines
@oerp-odoo
Copy link

Is this issue fixed?

Im still getting that warning on github actions. Should I just use # yamllint disable-line rule:truthy to disable it?

@brettinternet
Copy link

To fix this just add this to your .yamllint

rules:
  truthy:
    allowed-values: ['true', 'false']
    check-keys: false

briandfoy added a commit to briandfoy/brians_perl_modules_appveyor_config that referenced this issue Sep 1, 2022
@tshowers-bt
Copy link

tshowers-bt commented Oct 25, 2022

edit: The paragraph below seems to make a bad assumption about the way key text is interpreted. adrienverge I've now seen all the dupes and have a bit more context so I figured I'd I leave my original comment unedited for context and explain what else I came across below for any future readers. If you think this is just noise though we can delete it.

Is there a reason not to make check-keys: false the default? Warning about keys that would be truthy-but-not-idiomatic values if they were values instead of keys seems more like a bug than a feature. Recommending that something: on be replaced by something: true makes sense because they are semantically equivalent but the latter is the more idiomatic way to express it. Recommending that on: something be replaced by true: something seems strange because they're not equivalent and the change has nothing to do with truthiness. The effect of checking keys just seems to be an arbitrary ban on key names, one of which is very common in CI definitions. Maybe I'm misunderstanding the use case but I can't imagine why someone would want to be warned that they used one of these names for a key given that changing the key name isn't even an option if you're using a schema you don't control (like travis or github).


Well, after looking into this more it seems like keys are values that (sometimes?) follow the same rules as non-key values when they are interpreted. For example; I took and example from the yamllint docs that refers to keys being interpreted as bool and converted it to JSON using an online converter. It didn't result in y, yes, and on colliding but it did result in true and True colliding. It's not just case insensitivity though because on and On don't collide.

This can be useful to prevent surprises from YAML parsers transforming [yes, FALSE, Off] into [true, false, false] or {y: 1, yes: 2, on: 3, true: 4, True: 5} into {y: 1, true: 5}.

Conversions on https://onlineyamltools.com/convert-yaml-to-json

image

image

I still think the behavior with check-keys: true is surprising but now I'm thinking it's because the YAML spec is surprising (I'll have to dig into it now to be sure).

marccarre added a commit to marccarre/ssh-to-ansible that referenced this issue Nov 10, 2023
Otherwise, yamllint complains with:

```
2:1       warning  truthy value should be one of [false, true]  (truthy)
```

Alternative:
Adding the following comment in all GHA YAML files:

```
```

See also: adrienverge/yamllint#158 (comment)
adrienverge added a commit that referenced this issue Jan 31, 2024
Specification of YAML ≤ 1.1 has 18 boolean values:

    true  | True  | TRUE | false | False | FALSE
    yes   | Yes   | YES  | no    | No    | NO
    on    | On    | ON   | off   | Off   | OFF

Whereas YAML 1.2 spec recognizes only 6:

    true  | True  | TRUE | false | False | FALSE

For documents that explicit state their YAML spec version at the top of
the document, let's adapt the list of forbidden values.

In the future, we should:
- implement a configuration option to declare the default YAML spec
  version, e.g. `default-yaml-spec-version: 1.2`,
- consider making 1.2 the default in a future release (this would be a
  slight breaking change, but yamllint always tried to be
  1.2-compatible).
- consider adapting yamllint to other 1.1 vs. 1.2 differences [^2].

Solves: #587

Related to: #559 #540 #430 #344 #247 #232 #158

[^1]: https://yaml.org/spec/1.2.2/#1032-tag-resolution
[^2]: https://yaml.org/spec/1.2.2/ext/changes/#changes-in-version-12-revision-120-2009-07-21
adrienverge added a commit that referenced this issue Jan 31, 2024
Specification of YAML ≤ 1.1 has 18 boolean values:

    true  | True  | TRUE | false | False | FALSE
    yes   | Yes   | YES  | no    | No    | NO
    on    | On    | ON   | off   | Off   | OFF

Whereas YAML 1.2 spec recognizes only 6 [^1]:

    true  | True  | TRUE | false | False | FALSE

For documents that explicit state their YAML spec version at the top of
the document, let's adapt the list of forbidden values.

In the future, we should:
- implement a configuration option to declare the default YAML spec
  version, e.g. `default-yaml-spec-version: 1.2`,
- consider making 1.2 the default in a future release (this would be a
  slight breaking change, but yamllint always tried to be
  1.2-compatible).
- consider adapting yamllint to other 1.1 vs. 1.2 differences [^2].

Solves: #587

Related to: #559 #540 #430 #344 #247 #232 #158

[^1]: https://yaml.org/spec/1.2.2/#1032-tag-resolution
[^2]: https://yaml.org/spec/1.2.2/ext/changes/#changes-in-version-12-revision-120-2009-07-21
adrienverge added a commit that referenced this issue Feb 4, 2024
Specification of YAML ≤ 1.1 has 22 boolean values:

    y     | Y            | n     | N
    yes   | Yes   | YES  | no    | No    | NO
    true  | True  | TRUE | false | False | FALSE
    on    | On    | ON   | off   | Off   | OFF

Whereas YAML 1.2 spec recognizes only 6 [^1]:

    true  | True  | TRUE | false | False | FALSE

For documents that explicit state their YAML spec version at the top of
the document, let's adapt the list of forbidden values.

In the future, we should:
- implement a configuration option to declare the default YAML spec
  version, e.g. `default-yaml-spec-version: 1.2`,
- consider making 1.2 the default in a future release (this would be a
  slight breaking change, but yamllint always tried to be
  1.2-compatible).
- consider adapting yamllint to other 1.1 vs. 1.2 differences [^2].

Solves: #587

Related to: #559 #540 #430 #344 #247 #232 #158

[^1]: https://yaml.org/spec/1.2.2/#1032-tag-resolution
[^2]: https://yaml.org/spec/1.2.2/ext/changes/#changes-in-version-12-revision-120-2009-07-21
adrienverge added a commit that referenced this issue Feb 6, 2024
Specification of YAML ≤ 1.1 has 22 boolean values:

    y     | Y            | n     | N
    yes   | Yes   | YES  | no    | No    | NO
    true  | True  | TRUE | false | False | FALSE
    on    | On    | ON   | off   | Off   | OFF

Whereas YAML 1.2 spec recognizes only 6 [^1]:

    true  | True  | TRUE | false | False | FALSE

For documents that explicit state their YAML spec version at the top of
the document, let's adapt the list of forbidden values.

In the future, we should:
- implement a configuration option to declare the default YAML spec
  version, e.g. `default-yaml-spec-version: 1.2`,
- consider making 1.2 the default in a future release (this would be a
  slight breaking change, but yamllint always tried to be
  1.2-compatible).
- consider adapting yamllint to other 1.1 vs. 1.2 differences [^2].

Solves: #587

Related to: #559 #540 #430 #344 #247 #232 #158

[^1]: https://yaml.org/spec/1.2.2/#1032-tag-resolution
[^2]: https://yaml.org/spec/1.2.2/ext/changes/#changes-in-version-12-revision-120-2009-07-21
adrienverge added a commit that referenced this issue Feb 6, 2024
Specification of YAML ≤ 1.1 has 22 boolean values:

    y     | Y            | n     | N
    yes   | Yes   | YES  | no    | No    | NO
    true  | True  | TRUE | false | False | FALSE
    on    | On    | ON   | off   | Off   | OFF

Whereas YAML 1.2 spec recognizes only 6 [^1]:

    true  | True  | TRUE | false | False | FALSE

For documents that explicit state their YAML spec version at the top of
the document, let's adapt the list of forbidden values.

In the future, we should:
- implement a configuration option to declare the default YAML spec
  version, e.g. `default-yaml-spec-version: 1.2`,
- consider making 1.2 the default in a future release (this would be a
  slight breaking change, but yamllint always tried to be
  1.2-compatible).
- consider adapting yamllint to other 1.1 vs. 1.2 differences [^2].

Solves: #587

Related to: #559 #540 #430 #344 #247 #232 #158

[^1]: https://yaml.org/spec/1.2.2/#1032-tag-resolution
[^2]: https://yaml.org/spec/1.2.2/ext/changes/#changes-in-version-12-revision-120-2009-07-21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants