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

Switch to yaml.v2 as the YAML parser #511

Merged
merged 2 commits into from
Jul 26, 2017
Merged

Switch to yaml.v2 as the YAML parser #511

merged 2 commits into from
Jul 26, 2017

Conversation

keithpitt
Copy link
Member

To fix: #510

We use github.com/ghodss/yaml as our YAML parser, which in turn uses https://github.com/go-yaml/yaml. For some reason (probably me, but I can't remember) we're using a modified version of ghodss/yaml that uses cloudfoundry-incubator/candiedyaml instead.

Switching to the proper ghodss/yaml version seems to fix the YAML parsing issue as reported in #510. I need to do some more digging why I did it like this in the first place...

@keithpitt keithpitt requested review from sj26 and lox July 26, 2017 02:16
@keithpitt
Copy link
Member Author

Heh, turns out ghodss/yaml tried cloudfoundry-incubator/candiedyaml for a while, then switch back to the old version. I guess we added this package at the time they were trailing the new one :)

@sj26
Copy link
Member

sj26 commented Jul 26, 2017

I remember having a go (hah!) at this while back and it turned out more difficult than I thought... but I don't remember why. We should definitely update it, though!

@lox
Copy link
Contributor

lox commented Jul 26, 2017

go-yaml for sure!

@keithpitt keithpitt merged commit 728903e into master Jul 26, 2017
@keithpitt keithpitt deleted the switch-yaml-parser branch July 26, 2017 04:29
@stefanmb
Copy link
Contributor

@lox @sj26 @keithpitt Thank you, I can confirm this fixed the problem!

@keithpitt
Copy link
Member Author

@stefanmb excellent! I even wrote a test with your example to make sure we don't break it again. It's a good YAML example to make sure that our parser can handle the more gnarly scenarios :)

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

Successfully merging this pull request may close these issues.

Agent 3.x cannot handle valid YAML pipeline definitions
4 participants