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

Stop supporting optional tag:yaml.org,2002:bool to ease adoption for GitHub Actions Workflow templating #407

Open
m0un10 opened this issue May 29, 2021 · 7 comments
Labels
enhancement This issue is a feature request priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome. recommended sponsored for prioritization into Carvel maintainer's backlog surprising to users

Comments

@m0un10
Copy link

m0un10 commented May 29, 2021

What steps did you take:
Using on: in a yaml file

What happened:
The on: was replaced with true:

What did you expect:
As on: is a valid field it should not have been converted to true

Anything else you would like to add:
This is very strange and impacts the ability to create templates for github actions.

Environment:

  • 0.33.0
@m0un10 m0un10 added bug This issue describes a defect or unexpected behavior carvel triage This issue has not yet been triaged for relevance labels May 29, 2021
@pivotaljohn
Copy link
Contributor

Hey Craig.

Yes, this is an irritating behavior from what was intended to be an optional part of the YAML 1.1 spec.

👉🏻 An immediate workaround is to make such values explicitly strings by surrounding them in quotes:

name: CI
"on":
  push:
    branches: [ develop ]
  pull_request:
    branches: [ develop ]
jobs:
  build:
    runs-on: ubuntu-latest
...

...

Now, this was corrected in the YAML 1.2 spec.

However, the current version of the Go YAML package (v2.x of that library) attempted to be compatible with both the YAML 1.1 and YAML 1.2 specs and so continues to accept these synonyms for boolean.

It's been a long-standing issue that this behavior is an annoyance (see go-yaml/yaml#214 for details).

It has been addressed in the not-yet-released version (v3.x) of the the Go YAML package. Even when this version of the package is released, it will be a breaking change for ytt and would need to be scheduled as such.

@pivotaljohn pivotaljohn added helping with an issue Debugging happening to identify the problem and removed bug This issue describes a defect or unexpected behavior carvel triage This issue has not yet been triaged for relevance labels May 31, 2021
@pivotaljohn
Copy link
Contributor

Floating this:

  1. https://yaml.org/type/bool.html — is an optional portion of the YAML 1.1 spec (and removed in the YAML 1.2 spec)
  2. the implementation of which in the current version of the Go YAML Package used in ytt is isolated to an initializer.[1]
  3. as noted above, there are legit use cases for same strings (e.g. on) as strings.
  4. I have yet to see someone seriously use these aliases.

Which gets my mind to what @m0un10 is pointing out, here: that this optional implementation portion of the YAML spec is actively aggressive for a whole class of potential users.

What's the downside of introducing a breaking change where we simply no longer support these optional boolean aliases?


[1] https://github.com/vmware-tanzu/carvel-ytt/blob/faafacea467ccd661809f763d77bfb254574f86c/pkg/yamlmeta/internal/yaml.v2/resolve.go#L40-L45

@pivotaljohn pivotaljohn added discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution and removed helping with an issue Debugging happening to identify the problem labels Jun 8, 2021
@pivotaljohn pivotaljohn changed the title For some reason on: is changed to true: Stop supporting optional tag:yaml.org,2002:bool to ease adoption for GitHub Actions Workflow templating Jun 8, 2021
@m0un10 m0un10 closed this as completed Jun 11, 2021
@m0un10 m0un10 reopened this Jun 11, 2021
@github-actions github-actions bot added the carvel triage This issue has not yet been triaged for relevance label Jun 11, 2021
@m0un10
Copy link
Author

m0un10 commented Jun 11, 2021

sorry, I accidentally closed it (so re-opened it). I was just going to leave a comment to say that commenting out line 42 and rebuilding gives me a way forward for templating github actions workflows. It would be great if the aggressive bool tag renaming got removed in a future release though!

@pivotaljohn pivotaljohn removed the carvel triage This issue has not yet been triaged for relevance label Jun 15, 2021
@pivotaljohn
Copy link
Contributor

pivotaljohn commented Jun 15, 2021

Okay... I suspect no one is using any of the synonyms.

To avoid confusion, I'm suggesting that we support none of the synonyms.

Not:

  • on/On/ON
  • off/Off/OFF
  • y/Y/yes/Yes/YES
  • n/N/no/No/NO

and only support the true/false variants:

  • true/True/TRUE
  • false/False/FALSE

Querying the community: https://kubernetes.slack.com/archives/CH8KCCKA5/p1623796540037600

@pivotaljohn
Copy link
Contributor

This would addresses #146 (and the many (#189) many (#219) many (#275) subsequent reports of running into this issue).

@pivotaljohn pivotaljohn added enhancement This issue is a feature request priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome. recommended sponsored for prioritization into Carvel maintainer's backlog surprising to users and removed discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution labels Sep 20, 2021
@pivotaljohn
Copy link
Contributor

Getting absolutely no pushback on this issue. And we're getting a regular report of this being an irritant.

Let's do it.

@pivotaljohn
Copy link
Contributor

This one has yet to be prioritized and it caught another: #764 . sigh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a feature request priority/unprioritized-backlog Higher priority than priority/awaiting-more-evidence but not planned. Contributions are welcome. recommended sponsored for prioritization into Carvel maintainer's backlog surprising to users
Projects
Status: To Triage
Development

No branches or pull requests

2 participants