-
Notifications
You must be signed in to change notification settings - Fork 41
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
Avoid emitting YAML back-references in kpt fn output #183
Avoid emitting YAML back-references in kpt fn output #183
Conversation
b403b4f
to
91aafe7
Compare
@dflemstr I didn't know about refs before so thanks for bringing attention to them. I think back references fit very well with Don't Repeat Yourself principle. This seems like something that will come default in yaml parsers in 2-3 yrs, esp since js-yaml already defaults noRefs to false. WDYT? Is this accurate? If so, we should track an issue to revisit this before v1.0.0 release. |
@prachirp I agree that in principle anchors/back-references have some benefits in that they do aid in DRY. In practice, this seems to be one of the obscure features in the YAML spec that even many popular YAML parsers have a hard time supporting correctly. For example, one of our tools is using the Jackson library and is hitting this bug: FasterXML/jackson-dataformats-text#98 which seems to have affected other popular products in the ecosystem like Spinnaker/Keel and Elasticsearch. Despite many attempts to fix it, it seems that even the most popular Java YAML parsing libraries, Jackson and SnakeYAML, both still have trouble supporting this feature. I'm not sure if I'd expect this situation to change any time soon. And, especially since kpt function ResourceLists are supposed to be machine readable, I would vote for applying the Robustness Principle in this case! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dflemstr I see, thanks for the background! I agree, robustness comes first.
LGTM, but let's wait for @frankfarzan to take a look too.
@frankfarzan would you have a chance to take a look at this? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look good, just a minor comment.
@@ -403,13 +403,13 @@ export interface Result { | |||
*/ | |||
export type Severity = 'error' | 'warn' | 'info'; | |||
|
|||
interface JsonArray extends Array<Json> {} | |||
export interface JsonArray extends Array<Json> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All exported types need to be documented with a comment.
@frankfarzan done! |
bc93646
to
ff17d67
Compare
Avoid using YAML refs because many YAML parsers in the k8s ecosystem don't support them.
Without this change, a typical output from a kpt fn run might look like this:
The usage of
&ref_0
and*ref_0
breaks many YAML parsers that don't implement that YAML feature.