Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Relax yaml parsing in releases/view page #636

Closed
migmartri opened this issue Sep 13, 2018 · 2 comments · Fixed by #797
Closed

Relax yaml parsing in releases/view page #636

migmartri opened this issue Sep 13, 2018 · 2 comments · Fixed by #797
Assignees
Labels
component/ui Issue related to kubeapps UI kind/feature An issue that reports a feature (approved) to be implemented
Milestone

Comments

@migmartri
Copy link
Contributor

@stormmore reported that Kubeapps was failing to show the resources part of his chart.

The research showed that the root cause was a parsing error on our client.

Our yaml parser complained about a duplicated tag vs Helm does not. We should look into if there is any way to make our parser less restrictive. More taking into account that a duplicated label is not per-se forbidden or discouraged by the helm linter so it might happen again.

@migmartri migmartri added kind/feature An issue that reports a feature (approved) to be implemented component/ui Issue related to kubeapps UI priority/backlog labels Sep 13, 2018
@prydonius
Copy link
Contributor

prydonius commented Sep 13, 2018

We use the safeLoad function from https://github.com/nodeca/js-yaml#safeload-string---options-, and it looks like there is an option to be compatible with the JSON.parse behaviour which explicitly says "If true, then duplicate keys in a mapping will override values rather than throwing an error." So setting this to true should fix the reported issue, but we may want to also consider the non-safe load function as we know that Helm & Kubernetes have pre-validated the YAML we're reading. That said, I don't think we need the extra stuff load gives us, and compatibility with JSON.parse might be enough.

@prydonius
Copy link
Contributor

For some reason, likely due to a bug in the upstream package, yaml.safeLoadAll does not seem to correctly support the json option, though yaml.safeLoad and yaml.loadAll do. Once again, given that we are only going to be parsing YAML that has already been parsed by Helm and Kubernetes, I think it is okay to switch to yaml.loadAll here, which seems to support some special JS-related YAML tags: https://github.com/nodeca/js-yaml#load-string---options-.

Filed nodeca/js-yaml#456 for the upstream issue.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
component/ui Issue related to kubeapps UI kind/feature An issue that reports a feature (approved) to be implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants