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

[WIP] Support unmarshall multiple yaml documents #8313

Closed
wants to merge 3 commits into from

Conversation

krzyzacy
Copy link
Member

@krzyzacy krzyzacy commented Jun 9, 2018

looks like go-yaml/yaml#301 added the support already

(fighting with dep)

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 9, 2018
@fejta
Copy link
Contributor

fejta commented Jun 9, 2018

I do not think switching out the yaml package will go well

@BenTheElder
Copy link
Member

+1 to @fejta's comment. we can use the existing marshalling / unmarshalling and just have a separate step to split the file into byte blobs that are seperate documents. they're just delimited by ---\n

@BenTheElder
Copy link
Member

actual spec details on this: http://yaml.org/spec/1.2/spec.html#id2760395

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krzyzacy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 9, 2018
@krzyzacy
Copy link
Member Author

krzyzacy commented Jun 9, 2018

@fejta hummm, why?

I mean I can do what @BenTheElder has suggested, but why we ping to a specific version of yaml library? aka what can break?

@krzyzacy
Copy link
Member Author

krzyzacy commented Jun 9, 2018

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 9, 2018
@krzyzacy krzyzacy closed this Jun 11, 2018
@krzyzacy krzyzacy reopened this Jun 12, 2018
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 12, 2018
@k8s-ci-robot
Copy link
Contributor

@krzyzacy: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-test-infra-lint 79485cb link /test pull-test-infra-lint
pull-test-infra-bazel 79485cb link /test pull-test-infra-bazel

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@krzyzacy
Copy link
Member Author

/shrug
@stevekuznetsov forgot ghodss/yaml does this jsonToYaml thing, and they are not caught up with upstream go-yaml :-\

I'd prefer do the split ourselves (it's pretty straight forward) until they update the library

@k8s-ci-robot k8s-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Jun 12, 2018
@stevekuznetsov
Copy link
Contributor

Can we do the split as a patch on top of the vendored code? Then propose the changes upstream? There's no good reason we should work around the library instead of trying to improve it for all :)

@krzyzacy
Copy link
Member Author

@stevekuznetsov do we suppose to edit vendor code?

@krzyzacy
Copy link
Member Author

hummmmm seems it's not going to be easy if we want to use goyaml's decoder in ghodss/yaml

https://github.com/go-yaml/yaml/blob/v2/yaml.go#L95 the goyaml parser struct is unexported, and I don't really have a way to wrap around https://github.com/go-yaml/yaml/blob/v2/yaml.go#L119-L135 unless I write my own parser :-/

opened ghodss/yaml#31

@krzyzacy
Copy link
Member Author

/shrug
I see k8s already does that in https://github.com/kubernetes/apimachinery/blob/master/pkg/util/yaml/decoder.go, probably just use that instead

@krzyzacy krzyzacy closed this Jun 13, 2018
@krzyzacy
Copy link
Member Author

don't need to bump go-yaml, will switch to the other branch instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants