Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Flux automated workload creates invalid yaml #2003

Closed
ChSch3000 opened this issue May 1, 2019 · 4 comments
Closed

Flux automated workload creates invalid yaml #2003

ChSch3000 opened this issue May 1, 2019 · 4 comments

Comments

@ChSch3000
Copy link

Describe the bug
A clear and concise description of what the bug is.

To Reproduce
Steps to reproduce the behaviour:

  1. Create a workload
  2. Add flux.weave.works/automated: "true" annotation
  3. Update the corersponding docker image
  4. Wait until flux updates the wokload and sync the changes back to git
  5. Checkout newest comit
  6. Open the YAML in an editor

The IDE is complainng about an invalid yaml file, because of a missing value for the creationTimespamp key.

Expected behavior
The YAMLs must be valid after checkout, so creationTimesptamp Key should not be empty. Either it should be null or left unmodified by flux.

Additional context
Add any other context about the problem here, e.g

  • Flux version: 1.12.0
  • Kubernetes version: 1.13

image

@2opremio 2opremio added blocked-needs-validation Issue is waiting to be validated before we can proceed bug labels May 1, 2019
@stefanprodan
Copy link
Member

stefanprodan commented May 1, 2019

A workaround would be to remove the creationTimestamp from your YAMLs, that field represents the server time when an object was created, so it doesn't make sense to have it in git.

@2opremio 2opremio changed the title Flux atuomated wokload creates invalid yaml Flux automated workload creates invalid yaml May 1, 2019
@2opremio
Copy link
Contributor

2opremio commented May 1, 2019

What IDE is this? This seems to be a bug in the IDE's parser. Empty mapping value nodes are valid.

In YAML 1.2 :

YAML allows the node content to be omitted in many cases. Nodes with empty content are interpreted as if they were plain scalars with an empty value. Such nodes are commonly resolved to a “null” value.

In YAML 1.1:

# This mapping has four keys,
# one has a value.
empty:
canonical: ~
english: null
~: null key

So, Flux isn't generating an invalid document. However:

  • Flux should minimize (or ideally avoid altogether) unnecessary side-effects in the YAML files it edits.
  • The spec doesn't enforce parsing empty nodes as null. So, arguably, the output value isn't totally equivalent.

To update the file, Flux needs to parse it, edit the parsed representation and dump it again. Parsers are not perfect at preserving the initial YAML representation. We went to great lengths just to preserve YAML comments, writing a Python-based tool (kubeyaml) because, at the time, ruamel.yaml was the only solid library providing round-trip parsing.

In this particular case, what happens is that ruamel.yaml parses null as None and dumps None as empty. This generates a mostly-equivalent YAML representation, but causes a side-effect with respect to the original file. See https://bitbucket.org/ruamel/yaml/issues/169/roundtripdumper-dumps-null-values

We could generate null for everything parsed as None but that, again, this would generate side-effects when dumping things like empty documents.

Unfortunately, there is not much we can do besides improving the library (which we don't have the bandwidth for right now) or moving to a better parser

We are planning to move from kubeyaml to go-yaml.v3 (see #1933 ) but I doubt it does a much better job.

I have asked the author of ruamel.yaml about generating null only for empty mapping keys. If there's a way, that may be an acceptable fix.

@2opremio 2opremio added ⚽️ boot for now and removed blocked-needs-validation Issue is waiting to be validated before we can proceed labels May 1, 2019
@ChSch3000
Copy link
Author

@2opremio
Thank you for your explanation. Didn't know that empty values are valid in YAML.
I'm using Visual Studio Code, but the error comes from a Kubernetes extensions (couldn't figure out which one)
For now I can use the @stefanprodan's workaround

@squaremo
Copy link
Member

squaremo commented May 1, 2019

Thanks for the high quality bug report @ChSch3000 👍. I'm going to close this issue, on the basis that it is at least arguably correct behaviour (albeit one that caused your tooling problems); and, there is an acceptable workaround.

@squaremo squaremo closed this as completed May 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants