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

Consider changing truthy linter's check-keys default #559

Open
gravieure opened this issue Apr 4, 2023 · 6 comments
Open

Consider changing truthy linter's check-keys default #559

gravieure opened this issue Apr 4, 2023 · 6 comments

Comments

@gravieure
Copy link

The default configuration for the truthy linter applies it to keys as well as values. The syntax for GitHub Actions definitions uses an on key to determine what event the workflow applies to.

The upshot is that, out of the box, yamllint will blow up on any YAML document configuring a GitHub Action. This certainly can be fixed by disabling check-keys, but the presence of that option is non-obvious.

Given that GitHub Actions are a very common YAML usecase, do you think it'd be reasonable to disable this linter's check-keys option by default?

@adrienverge
Copy link
Owner

Hello, is it a duplicate of #540, #430, #344, #232, #247, #158 and others?

@gravieure
Copy link
Author

No; those all assume that the behavior is a bug needing to be fixed. I understand that it is not a bug, but an option which is enabled by default. My question is: would it make sense to change that default? The numerous issues filed on this subject are strong evidence that the default is confusing.

If changing the default is disagreeable, what do you think about the truthy linter printing some info about the check-keys option in this scenario? I think that would also reduce confusion and duplicate bug reports, since people would be guided to the right option without needing to file an issue about it.

@SubaruArai
Copy link

I was thinking to suggest using a %YAML 1.2 tag to automatically disable check-keys.
But then again, found out that pyyaml (still!) doesn't support yaml 1.2.

Switching to ruamel.yaml might be an option, but that's for another discussion.

@777arc
Copy link

777arc commented Aug 11, 2023

I think this is very reasonable, either disable check-keys or disable it for files inside of .github/workflows

@adrienverge
Copy link
Owner

TL;DR: For those seeking a quick-win to silent the error on GitHub Actions, here are 4 solutions:
- use config option rules: {truthy: {check-keys: false}}
- use config option rules: {truthy: {allowed-keys: [on, …]}}
- disable the check on the specific line with # yamllint disable-line rule:truthy
- quote the on key, e.g. "on"

No; those all assume that the behavior is a bug needing to be fixed. I understand that it is not a bug, but an option which is enabled by default. My question is: would it make sense to change that default? The numerous issues filed on this subject are strong evidence that the default is confusing.

I understand, but changing a default configuration in yamllint would break behavior in processes (e.g. continuous integration) for many users and companies. It is not an option to consider.

If changing the default is disagreeable, what do you think about the truthy linter printing some info about the check-keys option in this scenario? I think that would also reduce confusion and duplicate bug reports, since people would be guided to the right option without needing to file an issue about it.

This is interesting. But if I understand correctly, your proposition would produce extra "human-friendly" output amongst yamllint regular output (which has a constant pattern). I fear that this also would break tools relying on yamllint's output.

@sandstrom
Copy link

sandstrom commented Nov 16, 2023

I'd say we should rather lobby Github to update their workflows, to one of:

Suggesting a quoted 'on' or (even better) always interpret the workflow files as YAML 1.1 (maybe that's already what they're doing).

A temporary option for yamllint users:

  # .yamllint

  # don't use YAML's yes/no as boolean values (common cause of bugs)
  truthy:
    level: error

    # github action workflows use the key 'on' without quotes
    ignore: |
      .github/workflows/*.yml

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

No branches or pull requests

5 participants