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

yq creates a file it cannot parse by moving comments #427

Closed
jstangroome opened this issue Apr 28, 2020 · 10 comments
Closed

yq creates a file it cannot parse by moving comments #427

jstangroome opened this issue Apr 28, 2020 · 10 comments
Labels
Milestone

Comments

@jstangroome
Copy link

Describe the bug

Begin with yaml file with a comment placed within an array. Use yq to modify an unrelated document in the same file several times and watch the comment move each time until yq eventually fails with Error reading document...

Input Yaml
Concise yaml document(s) (as simple as possible to show the bug)
a.yaml:

rules:
  # comment
  - apiGroups: [""]
---
name: a

Command
The command you ran:

yq write -d1 a.yaml name b >b.yaml
yq write -d1 b.yaml name c >c.yaml
yq write -d1 c.yaml name d >d.yaml

Actual behavior

b.yaml

rules:
- # comment
  apiGroups: [""]
---
name: b

c.yaml

rules:
- apiGroups: # comment
[""]
---
name: c

and d.yaml fails to be created.

Expected behavior

d.yaml

rules:
  # comment
  - apiGroups: [""]
---
name: d

Additional context

Using: https://github.com/mikefarah/yq/releases/download/3.3.0/yq_linux_amd64

@mikefarah
Copy link
Owner

Yeah that's def dodgey - I noticed the comment placing before, but didn't realise it could result in what seems to be bad yaml.

Unfortunately this is handled by the underlying yaml parser yaml.v3 (https://github.com/go-yaml/yaml/tree/v3) so a fix will have to happen there, then I can simply bump the version and it will be fine.

@mikefarah
Copy link
Owner

Looks like this is related to go-yaml/yaml#587

@mikefarah
Copy link
Owner

and this go-yaml/yaml#459

@jstangroome
Copy link
Author

In the mean-time a workaround would be a mechanism to optionally strip the comments before modifying. yq write does not have the --stripComments argument like yq read does and I couldn't work out how to use yq read --stripComments to produce a new file while maintaining the multiple documents within.

@mikefarah
Copy link
Owner

Ah good one - I'll look into that

@mikefarah mikefarah added this to the 3.3.1 milestone Apr 29, 2020
@jstangroome
Copy link
Author

jstangroome commented May 1, 2020

This seems to be a workaround for stripping all the comments first but it isn't fun.

for doc in $(seq 0 "$(grep -c '^---' a.yaml)")
do
  printf -- '---\n'
  yq read --stripComments -d "${doc}" a.yaml
done >a2.yaml

Would be better if this worked without removing all the document headers (---), but this should probably become another GitHub issue:

yq read --stripComments --separateDocuments -d '*' a.yaml >a2.yaml

@niemeyer
Copy link

niemeyer commented May 4, 2020

go-yaml doesn't promise to preserve bit by bit the textual representation, so if that's the requirement we indeed won't be able to handle it in the short term. We may end up having a flag to strip comments, but meanwhile it should be quite straightforward to have a function that just zeros them out in the node structure.

@jstangroome
Copy link
Author

go-yaml doesn't promise to preserve bit by bit the textual representation, so if that's the requirement

Definitely not a requirement for my scenario. I'm not concerned if the comments move or get deleted, as long as the output yaml is still valid. I reported this bug only because the changing comments led to unparseable yaml.

@niemeyer
Copy link

niemeyer commented May 5, 2020

Can you please provide an example in a new issue outlining that case?

The existing ones in go-yaml are reorderings that preserve the data as-is, so if there's a case that breaks I want to look into it and fix it.

Thanks for your help.

@mikefarah
Copy link
Owner

@niemeyer

This takes a valid yaml and produces invalid yaml:

https://play.golang.org/p/wnFs49FLXep

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants