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

[lang] support emitting comments in resulting YAML #63

Open
cppforlife opened this issue Oct 17, 2019 · 38 comments
Open

[lang] support emitting comments in resulting YAML #63

cppforlife opened this issue Oct 17, 2019 · 38 comments
Labels
discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution enhancement This issue is a feature request lang support Support plain yaml syntax to reduce surprises and better integrate with other yaml tools

Comments

@cppforlife
Copy link
Contributor

originally @patricknelson added as a comment to related issue: #14 (comment):

It's not always essential for the resulting YAML to have comments for me, but it certainly would be very helpful!

What if we were to use a double # to work as an escape method when at the beginning of a line, so that the resulting YAML will contain a standard comment? For example, taken from the main demo at https://get-ytt.io:

#@ def labels():
#! This is a ytt comment
## This is a yaml comment
app: echo
org: test
#@ end

kind: Pod
apiVersion: v1
metadata:
  name: test-comments
  labels: #@ labels()

With the output resulting in:

kind: Pod
apiVersion: v1
metadata:
  name: echo-app
  labels:
    # This is a yaml comment
    app: echo
    org: test

I'm brand new to ytt (just doing research right now), so I'm asking just in case this isn't already available with first class syntax support without any special flags.

@cppforlife cppforlife added discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution enhancement This issue is a feature request labels Oct 17, 2019
@cppforlife
Copy link
Contributor Author

cppforlife commented Oct 17, 2019

It's not always essential for the resulting YAML to have comments for me, but it certainly would be very helpful!

that would be helpful in certain cases for sure.

What if we were to use a double # to work as an escape method when at the beginning of a line

that would work from a syntax perspective. minor concern i would have is if someone accidentally throws in annotations like ##@something thinking it's an annotation, but really would be annotation in a resulting YAML.

we currently use plain YAML library to implement serialization and it doesnt support emitting YAML comments; however, i was also playing around with ytt fmt command (experimental feature; #13) which has a custom YAML serializer that knows how to deal with comments. i did have some plans to switch to that one as it gets more mature.

@patricknelson
Copy link

That's entirely valid, especially since that's an easy typo to make. For me it'd be fine to emit a highly visible notice (or fatal parse error?) to STDERR when that special combination is encountered. Specifically, this notice would be triggered if the parser sees ##@ at the start of a new line.

I understand this is due to the ambiguity introduced from the [albeit positive] fact that YAML comments are used for ytt's DSL. Can you think of any other edge cases where this syntax could cause issues? Or alternatively, any better syntaxes? Perhaps this should be converted to an RFC for others to discuss (as I'm not the only user).

@cppforlife
Copy link
Contributor Author

cppforlife commented Oct 18, 2019

Or alternatively, any better syntaxes?

currently #! foo is a shorthand to #@comment foo. we could use something like #@yaml/comment foo to emit resulting yaml comments. this will fit nicely with other yaml/... annotations we already support: https://github.com/k14s/ytt/blob/develop/docs/lang-ref-annotation.md#yaml-templating-annotations

Perhaps this should be converted to an RFC for others to discuss (as I'm not the only user).

we currently dont have more advanced proposal process beyond dedicated github issues+comments (and slack for live discussion). imho it works ok for now.

@patricknelson
Copy link

Using #@yaml/comment foo sounds good to me from a structural/organizational standpoint. To make it more accessible (since that's still pretty verbose) can ## foo still be a shorthand for it? 😄 That's of course taking into account the validation against the potential ##@ typo.

@cppforlife
Copy link
Contributor Author

can ## foo still be a shorthand for it?

i would imagine it would be not be used that often, hence shorthand would not be necessary. how frequently do you think you'll be adding comments to resulting YAML?

@patricknelson
Copy link

Maybe not that often. My latest workflow is primarily focused on the ytt template files and not so much on the yaml that they generate. There may be others with workflows where they commit or otherwise retain long term the generated yaml with the intent for it to be readable later and, if that's the case, having comments will make sense.

So I cannot argue for it strongly since I may not use it much now, myself. Being a new user, I thought I might need it, but it turns out that it'll be of fairly low importance (and wouldn't use it at all if it had that verbose of syntax, actually).

@cppforlife
Copy link
Contributor Author

👍 let keep the issue open for now, and let's see if anyone else has a use case for this feature.

@smg8fe
Copy link

smg8fe commented Nov 7, 2019

Hi,
I'm trying to generate a cloud config file:
https://cloudinit.readthedocs.io/en/latest/topics/format.html#cloud-config-data
Its first line needs to be #cloud-config. Is there a way to achieve this now?

@cppforlife
Copy link
Contributor Author

@smg8fe for your use case you can use something like this for now, echo '#cloud-conifg'; ytt -f .

@PyMeH
Copy link

PyMeH commented Nov 27, 2019

Guys, yaml is yaml. IMO it would be best if ytt default behaviour does not fail when there is a comment in the input yaml file:

#@ def labels():
#! This is a ytt comment
## This is a comment in the yaml output
# And this is just a comment in this file

@cppforlife
Copy link
Contributor Author

@PyMeH our thinking was to help catch mistakes by authors if they forget to add @ after '#' (since IDEs for example dont help to add it). there is currently --ignore-unknown-comments to allow anything to pass through. do you think opting in for a flag is unnecessary?

@patricknelson
Copy link

patricknelson commented Nov 27, 2019

This makes me wonder @cppforlife, is there the ability to flag the file (maybe in the header at the very top) to enable this functionality? That or maybe some code-based configuration so that we can keep it "git ops". Since the code is already ytt capable (i.e. intentionally passed into ytt and likely including #@ directives and etc) then presumably someone who uses that special header/flag at the top of the file, and of course the --ignore-unknown-comments flag, will expect that functionality.

That way we can keep it all in-code and clearly defined as a preference somewhere.

@PyMeH
Copy link

PyMeH commented Nov 29, 2019 via email

@rnandi rnandi added the lang support Support plain yaml syntax to reduce surprises and better integrate with other yaml tools label Mar 20, 2020
@davedittrich
Copy link

@smg8fe for your use case you can use something like this for now, echo '#cloud-conifg'; ytt -f .

Please don't suggest hackish workarounds like this. I, too, am trying to do something more robust that Jinja templating to get values from external variables into a YAML file, in this case also a cloud-init that expects #cloud-config at the start of the file.

In this use case, I am trying to make it easier for someone with little programming experience to preconfigure a Raspberry Pi using a cloud-init file. I began using Jinja templating, which you yourself pointed out was inadequate in your IBM tech talk (where I learned about ytt in the first place.) Others are even more vocal about not using Jinja to template YAML. (https://leebriggs.co.uk/blog/2019/02/07/why-are-we-templating-yaml.html)

The concept of templating is hard enough, so new users insert hard-coded passwords into a YAML file, or worse, just use the default password that came in the cloned repo. It is already going to be a hard sell for me to get them to use ytt templating. Requiring additional scaffolding in a Makefile or extra scripts or complicated command lines to force an initial line in a file complicates the pipeline and the additional friction may scare them off completely. To me, that's a lose/lose situation.

I've been trying to find a supported way to include the string using existing ytt features (like text templating, etc.) While I think the text templating might be working, all attempts fail with the error message:

ytt: Error: Unmarshaling YAML template 'rpialgo.yml': yaml: line 4: did not find expected <document start>

I can only make ytt more strict with -s, but not less strict. Is it really necessary to take such a strongly opinionated view of what a "valid" YAML file is?

Not providing a means of working around this issue (besides concatenation at the shell) does not seem to be user-friendly or robust.

Can you just add an option to select what the "expected document start" string should be? I would be happy to just add --document-start '#cloud-config' to work around this.

Thanks!

@paulczar
Copy link

paulczar commented Feb 1, 2021

This would be super useful! We're starting to explore using ytt to process and create yaml documents that are meant for human consumption. Being able to pass comments through via ## would be fantastic so we can have inline documentation for the files created.

This exact use case is having a bunch of shorter yaml docs in source that can be composed together to create a valid values.yaml file for a specific environment for documentation purposes.

@jmcshane
Copy link
Contributor

The use case of using ytt for something like a cloud-config is especially challenging here. What is the appropriate way to engage here to get some focus on this use case from the maintainers here? I think there are valid use cases that would be very helpful for the project to consider here.

@dmarmugi
Copy link

see if anyone else has a use case for this feature

I found this issue looking for a way to tag my generated files with headers like

# generated by ytt, do not modify
# author: 
...

@voor
Copy link

voor commented Jul 31, 2021

I don't know if this belongs in here or could be a separate issue, but we're finding ourselves doing more and more stuff like this:

apiVersion: v1
kind: Secret
metadata:
  name: tbs-values
  namespace: kapp-controller
stringData:
  overlay-options.yaml: |
    #@ load("@ytt:overlay", "overlay")
    #@ load("@ytt:data", "data")

    #@ def sync_canonical_secret():
    apiVersion: v1
    kind: Secret
    metadata:
      name:  canonical-registry-secret
    #@ end

    #@overlay/match by=overlay.subset(sync_canonical_secret()),when="1+"
    ---
    metadata:
      #@overlay/match missing_ok=True
      labels:
        #@overlay/match missing_ok=True
        com.vmware.tanzu.buildservice.sync: "true"

Which is cutting down on readability pretty significantly, it would be really nice to be able to pull this out into a yaml fragment like we do with non-ytt overlay config files:

#@ def dex_config():
issuer: #@ "https://login.sso.{}".format(config.domain)
frontend:
  theme: tkg
web:
  http: 0.0.0.0:5556
...
#@ end

stringData:
  config.yaml: #@ yaml.encode(dex_config())

@jldec
Copy link

jldec commented Sep 23, 2021

If I understand this correctly, there is currently no way to force ytt to emit yaml # comments into the output yaml. For the use case where ytt is being used to generate new human-readable yaml this is a problem.

E.g. App Accelerator templates can invoke ytt on input yaml files, to generate modified output yaml files for a new project. The value of those output files is significantly impacted by a lack of comments.

@vrabbi
Copy link

vrabbi commented Sep 23, 2021

This would be a huge benefit for us to have this option. We have a few use cases currently impacted by this. The main one is using ytt in app accelerator and we need comments which isnt possible. The other is a bit more advanced but is using ytt to generate ytt templates. This also may be relevant down the road in a system like app accelerator but currently i use a bunch of sed and awk stuff to generate ytt templates that could much more easily and cleanly be done via ytt generating them.
As ytt is # based we need to be able to print out ytt directives in the output of the original ytt

@voor
Copy link

voor commented Sep 23, 2021

We try to bootstrap new repositories with templates that follow common patterns as "getting started" -- we do this mostly with sed scripts now, but it comes up extremely often it seems silly to use sed to create ytt templates when ytt would excel in that use case.

@patricknelson
Copy link

It stinks that we're still stuck at not being able to simply emit comments in ytt output.

@pivotaljohn what do you think: Could we at the very least have some kind of esoteric syntax like @cppforlife's suggestion above #63 (comment) just to make it possible? e.g. #@yaml/comment foo. Presumably this is fully backward compatible first class ytt syntax that shouldn't be a breaking change and could even be a minor/dot release.

... and then maybe discuss having a more intuitive shorthand syntax like ## foo (hash-hash-space)? 😉😉 nudge nudge...

@patricknelson
Copy link

This may be sacrilege, but: I’m actually dropping ytt entirely now in favor of kustomize since I’m finding that it’s actually much better suited for my use case anyway (building complex Kubernetes manifests). If anybody here is encountering issues while using ytt and happen to have a similar use case, I’d encourage you to give kustomize a shot as well. It’s just a completely different approach, but potentially much simpler (i.e. use of declarative overlays/components/generators instead of templates with parameterization and side-effects).

Ironically, it doesn’t include comments in resulting YAML, but it’s generally not necessary at all. 😊

@pivotaljohn
Copy link
Contributor

Hey @patricknelson. Sorry for the delay. Thanks for letting us know where you're at.

It's unfortunate that we haven't been able to make more progress on this issue. We're constantly prioritizing where we invest and haven't had the bandwidth to include this (albeit useful) suggestion.

We're really big on folks using the right tool for the job. 👍🏻 Kustomize can be great for straight-forward patching and is well integrated into the Kubernetes tooling.

ytt itself specifically includes both templating and overlaying/patching capabilities deliberately to cover a wider range of use-cases:

  • overlays are great for aspects of configuration (e.g. ensure all resources have a particular namespace; all containers have certain resource limits; etc.)
  • overlays allow the cluster operator to make "last minute" adjustments to k8s manifests
  • ytt overlays are written in the same structure as the documents they target (other tools use JSON Patch syntax)
  • templates tend to be easier to reason about since they explicitly indicate where values are being inserted
  • data values can be thought of as a formal API of anticipated configuration values.

Keeping this open as we haven't decided not to implement this feature. In the meantime, we would absolutely welcome a contribution for implementing the @yaml/comment annotation.

@pivotaljohn
Copy link
Contributor

@vrabbi noted, earlier:

This would be a huge benefit for us to have this option. We have a few use cases ... using ytt to generate ytt templates. ... currently i use a bunch of sed and awk stuff to generate ytt templates that could much more easily and cleanly be done via ytt generating them.

Another concrete example of this arose, recently: where a template evaluated at build time...

A Carvel user wanted to template their PackageInstall CR so that they could include it in a collection of packages to be installed as a larger system:

Build Step: during "build time", populate some fields with values determined by the published of the package:

telemetry-package-install.yml

#@ load("@ytt:data", "data")

#@ #@ load("@ytt:data", "data")
#@ #@ load("@ytt:yaml", "yaml")
#@ #@ load("_profiles.star", "profiles")

#@ #@ if profiles.is_any_enabled([profiles.full]):
#@ #@ if profiles.is_pkg_enabled("telemetry.platform.com"):

---
apiVersion: packaging.carvel.dev/v1alpha1
kind: PackageInstall
metadata:
  name: #@ data.values.name
  namespace: platform-install
spec:
  serviceAccountName: #@ data.values.serviceAccount
  packageRef:
    refName: #@ data.values.name + "." + data.values.domain
    versionSelection:
      constraints: #@ data.values.version
      prereleases: {}
---
apiVersion: v1
kind: Secret
metadata:
  name: telemetry-values
  namespace: platform-install
stringData:
  values.yml: #@ #@ yaml.encode(data.values.telemetry)

#@ #@ end
#@ #@ end

... such that the lines that start with #@ #@ ... are desired to pass through to the output:

#@ load("@ytt:data", "data")
#@ load("@ytt:yaml", "yaml")
#@ load("_profiles.star", "profiles")

#@ if profiles.is_any_enabled([profiles.full]):
#@ if profiles.is_pkg_enabled("telemetry.platform.com"):

---
apiVersion: packaging.carvel.dev/v1alpha1
kind: PackageInstall
metadata:
  name: telemetry
  namespace: platform-install
spec:
  serviceAccountName: telemetry-acct
  packageRef:
    refName: telemetry.platform.com
    versionSelection:
      constraints: 1.1.0
      prereleases: {}
---
apiVersion: v1
kind: Secret
metadata:
  name: telemetry-values
  namespace: platform-install
stringData:
  values.yml: #@ yaml.encode(data.values.telemetry)

#@ end
#@ end

Install Step: while the system is being installed, the system operator provides installation values to complete the PackageInstall CR:

---
apiVersion: packaging.carvel.dev/v1alpha1
kind: PackageInstall
metadata:
  name: telemetry
  namespace: platform-install
spec:
  serviceAccountName: telemetry-acct
  packageRef:
    refName: telemetry.platform.com
    versionSelection:
      constraints: 1.1.0
      prereleases: {}
---
apiVersion: v1
kind: Secret
metadata:
  name: telemetry-values
  namespace: platform-install
stringData:
  values.yml: |
    apiVersion: observability/v1alpha1
    kind: TelemetryConfig
    spec: 
      ...

@pivotaljohn
Copy link
Contributor

A potentially related example: the ability to — from within a template — take a YAML Fragment and encode it (complete with its annotations) ... Being able to do so means that such values (e.g. a k8s Secret who's data is an overlay) could be closer to being editable (overlay-able) as well... 🤔

https://kubernetes.slack.com/archives/CH8KCCKA5/p1652468285086519?thread_ts=1652465480.622579&cid=CH8KCCKA5

@voor
Copy link

voor commented Nov 3, 2022

Friendly bump, this still is strongly desired.

@c33s
Copy link

c33s commented Jan 27, 2023

i would vote for simply allowing # as comment
see my comment here #14 (comment)

@voor
Copy link

voor commented Feb 2, 2023

i would vote for simply allowing # as comment see my comment here #14 (comment)

While this is a good point, the problem with the above examples is that we don't want things to go through as:

# #@ data.values.hello

and come out as:

# #@ data.values.hello

but instead to come out as:

#@ data.values.hello

in the output.

@patricknelson
Copy link

Just calling this out in abstract, but: What if we resolve some of this ambiguity issues by making a decision and then issuing warnings to the console when the application detects possible edge cases one the ones being called out for each various approach? Then maybe just add a flag maybe in the file to ignore that warning (e.g. sort of like what we already do for code linters).

Just a thought.

@gberche-orange
Copy link

gberche-orange commented Feb 3, 2023

let's see if anyone else has a use case for this feature.

Another example is ability to generate yaml comments interpreted by IDEs such as the # nonk8s comment supported by intellij (see related https://youtrack.jetbrains.com/issue/IDEA-308353/Kubernetes-document-nonk8s-directive ) which I use to tell the IDE to not consider kuttl files as K8S files.

The following ytt output is performed using data.load, however lack of comments rendering would break such approach then using a ytt library instead (see related thread https://kubernetes.slack.com/archives/CH8KCCKA5/p1673719852280819 )

apiVersion: v1
kind: ConfigMap
metadata:
  name: kuttle-test-files
  annotations:
    kapp.k14s.io/versioned: ""
    kapp.k14s.io/num-versions: "5"
data:
  kuttl-test-suite_00-all-tests.yaml: |-
    # nonk8s
    # See https://youtrack.jetbrains.com/issue/IDEA-307431/Kubernetes-Add-ability-to-suppress-k8s-schema-inspections-locally-by-comment#focus=Comments-27-6695586.0-0
    apiVersion: kuttl.dev/v1beta1
    kind: TestSuite
    metadata:
      name: 00-all-tests
    testDirs:
      - /kuttl-test-suite/01-sample-static-pg
      - /kuttl-test-suite/02-dynamic-small-pg
    timeout: 3600
    parallel: 1
    suppress:
      - "events"
    namespace: "75-crossplane-sample-xrcs"

@c33s
Copy link

c33s commented Feb 3, 2023

@voor
as i am still new to ytt i maybe misunderstand the benefit here. you wrote the output is #@ data.values.hello in the resulting yaml file.
as far is i understand, the resulting yaml file won't be parsed again, so what exactly is the benefit if having #@ data.values.hello vs # #@ data.values.hello in the output file? isn't this a complete cosmetic issue (sorry again if i make a newbie mistake in my thoughts)?
if it is really the case that we are talking about a cosmetic issue i would stand with my request/vote that ytt should not touch comments and keep them as is. why sacrify the option to simply use existing yaml file for a cosmetic feature?

@vrabbi
Copy link

vrabbi commented Feb 3, 2023

@c33s its not a cosmetic issue in many use cases. There are alot of cases i have had which im sure @voor alsi has encountered where i want to use ytt to output a ytt template. Think of an automation to produce ytt configurations.
For me this is especially needed when using tools like cartographer which allow for ytt templating inline the relevant CRs but i have the need to use ytt to configure these templates so having ytt be able to emit ytt files is a needed feature.
Another key use case for emitting ytt templates is when automating package generation which runs into the same issues as above.

@c33s
Copy link

c33s commented Feb 3, 2023

@vrabbi doesn't it work to use #@ data.values.hello instead of # #@ data.values.hello?

but in the end i think it should be configurabe

@vrabbi
Copy link

vrabbi commented Feb 3, 2023

@vrabbi doesn't it work to use #@ data.values.hello instead of # #@ data.values.hello?

but in the end i think it should be configurabe

No because then it would be parsed already by the initial ytt invocation

@vrabbi
Copy link

vrabbi commented Feb 3, 2023

Because for good and for bad ytt use the # symbol in its logic, i believe being explicit is needed. Part of the pain but huge benefit of ytt is that it is safe in nature unlike other tools using mechanisms like go templating. I feel the pain of not supporting plain # symbols but that is a byproduct of the core design of ytt which would be a complete breaking change if that were to change in my mind. Allowing the output of a plain # comment can be done i believe in the ideas put together by john on hackmd using the #@yaml/comment annotation which i think is a good option. You can also have ytt ignore comments in existing yaml by using the relevant flag when invoking ytt so as much as i feel the pain, i personally dont think that it is the right approach, i think being explicit is important, supporting emitting ytt templates and yaml with standard comments while not causing some really hard to debug issue is the critical aspect

@drawsmcgraw
Copy link

Just a simple comment to show support for the #@yaml/comment annotation. It's explicit and seems to be the least likely to cause token collisions with yaml files (we can't predict what people are going to put in their input files).

@carlosonunez-vmw
Copy link

+1 to @drawsmcgraw and @voor's comments for support. This would be a great nice-to-have.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution enhancement This issue is a feature request lang support Support plain yaml syntax to reduce surprises and better integrate with other yaml tools
Projects
Status: To Triage
Development

No branches or pull requests