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

Harden and simplify yaml stream decoding/encoding #1931

Merged
merged 4 commits into from Apr 15, 2019

Conversation

@2opremio
Copy link
Collaborator

commented Apr 12, 2019

Flux choked on end-of-document markers (...).

To avoid complicating the existing multidoc parser (stolen from kubernetes) I abused go-yaml'sDecoder to obtain the raw documents from the stream by unmarshalling to an interface{} and marshalling again.

Also, I replaced our ad-hoc stream parsing and generation by a go-yaml Decoder and Encoder.

I, however, left the encoding part of fluxctl save untouched since yaml.Encoder generates different output (no start-of-document prefix) and users may be relying on it.

During testing I made sure that changes I made in fluxd's Export() are compatible with fluxctl save prior to this change.

Fixes #1925

@2opremio 2opremio requested a review from hiddeco Apr 12, 2019

@2opremio 2opremio changed the title Support contigous yaml EOD markers Support contigous yaml SOD markers Apr 12, 2019

@2opremio 2opremio force-pushed the 2opremio:1925-contigous-yaml-EOD branch from 7e6125b to 395cf48 Apr 12, 2019

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 12, 2019

Uhm, I think I got something wrong. The test seems to pass without the code changes ...
(I would had sworn it didn't)

@2opremio 2opremio changed the title Support contigous yaml SOD markers [WIP] Support contigous yaml SOD markers Apr 12, 2019

@2opremio 2opremio force-pushed the 2opremio:1925-contigous-yaml-EOD branch from 395cf48 to d051c92 Apr 13, 2019

@2opremio 2opremio changed the title [WIP] Support contigous yaml SOD markers [WIP] Harden and simplify yaml stream parsing Apr 13, 2019

@2opremio 2opremio force-pushed the 2opremio:1925-contigous-yaml-EOD branch 2 times, most recently from 9e0ed2c to cf189e2 Apr 13, 2019

@2opremio 2opremio changed the title [WIP] Harden and simplify yaml stream parsing Harden and simplify yaml stream parsing Apr 13, 2019

2opremio added a commit to 2opremio/kubeyaml that referenced this pull request Apr 13, 2019
Make sure no end-of-document is produced
Technically it's correct to add an end-of-document mark to
the output. However, `kubectl` and `Flux` (before
fluxcd/flux#1931 ) choke on it.
2opremio added a commit to 2opremio/kubeyaml that referenced this pull request Apr 13, 2019
Make sure no end-of-document marker (`...`) is output
Technically it's correct to add an end-of-document mark to the output. However,
`kubectl` and `Flux` (before fluxcd/flux#1931 )
choke on it.

This change makes sure we always use `---` markers to separate documents and
that we never add `...`.
2opremio added a commit to 2opremio/kubeyaml that referenced this pull request Apr 13, 2019
Make sure no end-of-document marker (`...`) is put out
Technically it's correct to add an end-of-document mark to the output. However,
`kubectl` and `Flux` (before fluxcd/flux#1931 )
choke on it.

This change makes sure we always use `---` markers to separate documents and
that we never add `...`.
2opremio added a commit to 2opremio/kubeyaml that referenced this pull request Apr 13, 2019
Make sure no end-of-document marker (`...`) is written
Technically it's correct to add an end-of-document mark to the output. However,
`kubectl` and `Flux` (before fluxcd/flux#1931 )
choke on it.

This change makes sure we always use `---` markers to separate documents and
that we never add `...`.
2opremio added 2 commits Apr 12, 2019
Harden yaml stream parsing of manifests files
Flux choked on end-of-document markers (`...`).

To avoid complicating the existing multidoc parser (stolen from kubernetes) I
abused go-yaml's Decoder to obtain the raw documents from the stream by
unmarshalling to an interface{} and marshalling again.
Harden yaml stream decoding in `fluctl save`
Use go-yaml's Decoder instead of our own

@2opremio 2opremio force-pushed the 2opremio:1925-contigous-yaml-EOD branch from cf189e2 to 8edd13f Apr 13, 2019

@2opremio 2opremio changed the title Harden and simplify yaml stream parsing Harden and simplify yaml stream decoding/encoding Apr 14, 2019

@2opremio 2opremio requested a review from hiddeco Apr 14, 2019

@2opremio 2opremio force-pushed the 2opremio:1925-contigous-yaml-EOD branch from 2a47817 to abe9db8 Apr 14, 2019

@hiddeco
Copy link
Member

left a comment

Thanks Fons 🍪

@2opremio 2opremio merged commit 5ee497b into fluxcd:master Apr 15, 2019

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: e2e-testing Your tests passed on CircleCI!
Details

@2opremio 2opremio deleted the 2opremio:1925-contigous-yaml-EOD branch Apr 15, 2019

@squaremo

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

Q: Does this mean comments are no longer preserved when files are updated by flux?
(Genuine question; I have lost track of how the bytes are obtained and passed to kubeyaml)

@squaremo

This comment has been minimized.

Copy link
Member

commented Apr 24, 2019

A: no, because the kubeyaml-using code still just reads a whole file in, and writes a whole file out.

@squaremo squaremo added this to the v1.12.1 milestone Apr 24, 2019

@2opremio

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 24, 2019

Yep, exactly, sorry I didn't get to answer the question in time.

squaremo added a commit to squaremo/kubeyaml that referenced this pull request Apr 24, 2019
Make sure no end-of-document marker (`...`) is written
Technically it's correct to add an end-of-document mark to the output. However,
`kubectl` and `Flux` (before fluxcd/flux#1931 )
choke on it.

This change makes sure we always use `---` markers to separate documents and
that we never add `...`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.