Skip to content

fix: Update PrintNamespacesList to use a YAML parser for multiple manifest files #9825

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.

Sign up for GitHub

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

Jump to bottom
Merged
ChrisGe4 merged 14 commits into from
Jun 11, 2025
Merged

fix: Update PrintNamespacesList to use a YAML parser for multiple manifest files #9825

ChrisGe4 merged 14 commits into from
Jun 11, 2025

Conversation

@katiexzhang
Copy link
Contributor

@katiexzhang katiexzhang commented Jun 4, 2025 •
edited
Loading

Multiple manifests may be defined in a single YAML file. --- is used to separate the different manifests.

Before:

  • strings.Split("---") is used to break the file into multiple manifests. If a string in the manifest contains "---" (i.e. "name: hello---123"), it will also break on that and won't be able to decode the manifest anymore.

After:

Note: If a file just contains "---" it is considered empty and therefore valid and will just return an empty resourceToInfoMap.

@katiexzhang katiexzhang requested a review from a team as a code owner June 4, 2025 17:43
@katiexzhang katiexzhang changed the title Update PrintNamespacesList to break on YAML file separator '---' followed by a new line. fix: Update PrintNamespacesList to break on YAML file separator '---' followed by a new line. Jun 4, 2025
@plumpy
Copy link
Collaborator

plumpy commented Jun 4, 2025

What about

yaml: "
  ---
"

@plumpy
Copy link
Collaborator

plumpy commented Jun 4, 2025

Also YAML explicitly allows \r\n as a valid line break (for Windows compatibility): https://yaml.org/spec/1.2.2/#54-line-break-characters

@katiexzhang
Copy link
Contributor Author

katiexzhang commented Jun 4, 2025

What about

yaml: "
  ---
"

Right now this returns an error like:
2025/06/04 18:42:37 couldn't get version/kind; json parse error: json: cannot unmarshal string into Go value of type struct { APIVersion string "json:\"apiVersion,omitempty\""; Kind string "json:\"kind,omitempty\"" }

A file with just --- is technically empty so according to the comment "skip empty documents, Decode will fail on them" we should just skip it (and return nothing / no error). But, it is weird to have a manifest with just --- right?

@plumpy
Copy link
Collaborator

plumpy commented Jun 4, 2025

Sorry, that was a confusing example, but my point was that this is technically a valid YAML file:

foo: "
   ---
"

If you parse it, you get a map: {"foo": " --- "}. But your code is just going to split on ---\n and rip it into two different documents (neither of which is valid YAML).

My larger point is that trying to parse any structured format with a basic text-processing tools (like strings.Split) is not going to work very well. Just quickly perusing the spec, I can find all kinds of other ways to make valid YAML that won't work with your code. (You're allowed to have whitespace and comments and a variety of other things after the ---, for example; it doesn't need to be immediately followed by a newline.)

In short: if you want to parse YAML, use a YAML parser.

@pull-request-size pull-request-size bot added size/L and removed size/XS labels Jun 4, 2025
@katiexzhang
Copy link
Contributor Author

katiexzhang commented Jun 5, 2025

Ok, updated to use https://pkg.go.dev/gopkg.in/yaml.v3 for parsing the multiple files, but kept the kubernetes file parser.

So for the file with just "---" --> should that be valid then?

@katiexzhang katiexzhang requested a review from Darien-Lin June 5, 2025 15:52
@plumpy
Copy link
Collaborator

plumpy commented Jun 5, 2025

Yeah, I think this looks great, thanks.

@katiexzhang katiexzhang changed the title fix: Update PrintNamespacesList to break on YAML file separator '---' followed by a new line. fix: Update PrintNamespacesList to use a YAML parser for multiple manifest files Jun 5, 2025
@katiexzhang katiexzhang requested review from ChrisGe4 and removed request for Darien-Lin June 10, 2025 18:27
ChrisGe4
ChrisGe4 reviewed Jun 10, 2025
View reviewed changes
@ChrisGe4 ChrisGe4 merged commit 2648bec into GoogleContainerTools:main Jun 11, 2025
18 of 20 checks passed
plumpy pushed a commit that referenced this pull request Jun 17, 2025
@katiexzhang @plumpy
fix: Update PrintNamespacesList to use a YAML parser for multiple man…
4ac92ea 
…ifest files (#9825)

* Update PrintNamespacesList to break on YAML file separator '---' followed by a new line.

* fix: Update PrintNamespacesList to break on YAML file separator '---' followed by a new line.

* fix: update yaml line break character and add more tests

* fix: use yaml parser to process multiple files

* fix: add test for single manifest w break at end

* fix: remove unnecessary conversion

* fix: update import

* fix: if resourceToInfoMap is empty, return nil

* fix: format

* fix: log fmt write error and update empty manifest (with just ---) handling

* fix: format

* fix: fix imports

* fix: add test case
plumpy pushed a commit to plumpy/skaffold that referenced this pull request Jun 17, 2025
@katiexzhang @plumpy
fix: Update PrintNamespacesList to use a YAML parser for multiple man…
fe2fbb6 
…ifest files (GoogleContainerTools#9825)

* Update PrintNamespacesList to break on YAML file separator '---' followed by a new line.

* fix: Update PrintNamespacesList to break on YAML file separator '---' followed by a new line.

* fix: update yaml line break character and add more tests

* fix: use yaml parser to process multiple files

* fix: add test for single manifest w break at end

* fix: remove unnecessary conversion

* fix: update import

* fix: if resourceToInfoMap is empty, return nil

* fix: format

* fix: log fmt write error and update empty manifest (with just ---) handling

* fix: format

* fix: fix imports

* fix: add test case
plumpy added a commit that referenced this pull request Jun 18, 2025
@plumpy @katiexzhang
fix: Update PrintNamespacesList to use a YAML parser for multiple man…
95fb592 
…ifest files (#9825) (#9833)

* Update PrintNamespacesList to break on YAML file separator '---' followed by a new line.

* fix: Update PrintNamespacesList to break on YAML file separator '---' followed by a new line.

* fix: update yaml line break character and add more tests

* fix: use yaml parser to process multiple files

* fix: add test for single manifest w break at end

* fix: remove unnecessary conversion

* fix: update import

* fix: if resourceToInfoMap is empty, return nil

* fix: format

* fix: log fmt write error and update empty manifest (with just ---) handling

* fix: format

* fix: fix imports

* fix: add test case

Co-authored-by: Katie Zhang <katiexzhang@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChrisGe4 ChrisGe4 ChrisGe4 approved these changes

Assignees

No one assigned

Labels

size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@katiexzhang @plumpy @ChrisGe4