fix(package parser): only drop fully commented files #548
Conversation
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
pkg/parser/parser.go
Outdated
| @@ -130,19 +130,16 @@ func (p *PackageParser) Parse(_ context.Context, reader io.ReadCloser) (*Package | |||
| return pkg, nil | |||
| } | |||
|
|
|||
| // cleanYAML cleans up YAML by removing empty and commented out lines which | |||
| // cleanYAML cleans up YAML only drops fully commented inputs which would | |||
There was a problem hiding this comment.
This is reading a little off now - is it correct to say the following? I'm trying to be more complete about what this function is doing and returning - feel free to disregard the completeness aspect if its wrong or you don't think its needed, but the sentence as is is missing some flow 🤓
| // cleanYAML cleans up YAML only drops fully commented inputs which would | |
| // cleanYAML cleans up the input YAML by removing all leading and trailing white | |
| // space from each line. If the input yaml is completely commented, then nil | |
| // will be returned to avoid issues with decoding. |
There was a problem hiding this comment.
Nono, I'm not dropping commented lines, i am ONLY dropping the whole file if it's content is only composed of commented lines (or separators).
The current implementation was dropping all lines starting with a # considering them as comments, but that's not always true, for example that's not the case if a line is part of a multiline string. Properly removing commented lines would require a context-aware parser, but given that the change was made just to avoid fully empty files that would crash the decoder, there is no need to get into so much complexity.
| args: args{in: []byte("")}, | ||
| want: want{out: nil}, | ||
| }, | ||
| "CommentedOut": { |
There was a problem hiding this comment.
good test cases! one thing that is nagging my mind here is if there's a scenario @muvaf was handling with the original function that really needed all commented lines removed. It seems like the conclusion here is that that was too aggressive, and its OK to leave in commented lines if the full block isn't completely commented.
You feel good about that too? There is not a scenario where commented lines should actually be removed? 🤔
There was a problem hiding this comment.
From the description of the original PR my understanding was that only fully commented files were an issue, so being less aggressive should be fine. The current behavior in released versions of Crossplane is to not drop any line at all, so I guess there should be no issue only dropping fully commented files.
There was a problem hiding this comment.
@phisco is right, my motivation was to have the parser skip fully-commented documents so we don't have errors in the upstream marshal/unmarshal functions. That's why I added only fully-commented doc cases to the unit tests in the original PR. It was essentially extending the definition of empty document from having no characters at all to having all lines commented out. Not sure why I ended up removing any lines, maybe while trying to reduce the number of loops?
I think this function can just be a check like isEmpty and not make any changes to the document - the problem isn't about having comments but more about having the whole document not have any useful data hence failing the functions trying to cast it onto a struct.
There was a problem hiding this comment.
Thanks @muvaf for the feedback, I'll rename the function to IsEmpty and make sure it doesn't have any side effect 👍
pkg/parser/parser.go
Outdated
| // We don't want to return an empty document with only separators that | ||
| // have nothing in-between. | ||
| if empty && trimmed != "---" && trimmed != "..." { | ||
| if empty && trimmed != "" && trimmed != "---" && trimmed != "..." && !strings.HasPrefix(trimmed, "#") { |
There was a problem hiding this comment.
why do we do anything like that on the text level? Can't we just decode and see that a null object (no keys) comes out? Would be much safer than any complicated risky logic here.
There was a problem hiding this comment.
We use a high level Decode function from upstream which does not have a specific error type for empty documents as far as I remember (@phisco might know better now) and checking only for null would not be specific enough due to number of reasons it can be null.
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
Description of your changes
Fixes crossplane/crossplane#4631
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
Added unit tests and updated existing tests.