odo add binding drops all YAML comments from the Devfile

[rm3l]

/kind bug

What versions of software are you using?

Operating System:
Fedora release 36 (Thirty Six)

Output of odo version:
odo v3.0.0-alpha2 (ba7f1a1)

How did you run odo exactly?

❯ odo init --name my-sample-go --devfile go --starter go-starter

❯ git init && git add -A . && git commit -m "initial commit"

Now add a few comments in the devfile.yaml file.
This is just an example, but we can imagine users having more-complex Devfiles with many comments/documentation in them.

diff --git a/devfile.yaml b/devfile.yaml
index d39ff93..af9a6cd 100644
--- a/devfile.yaml
+++ b/devfile.yaml
@@ -1,5 +1,7 @@
+# User comments in the Devfile will get removed by running 'odo add binding'
 commands:
 - exec:
+    # Description of the build command
     commandLine: GOCACHE=${PROJECT_SOURCE}/.cache go build main.go
     component: runtime
     group:
@@ -8,6 +10,7 @@ commands:
     workingDir: ${PROJECT_SOURCE}
   id: build
 - exec:
+    # Description of the run command
     commandLine: ./main
     component: runtime
     group:

❯ git commit -am "Add comments to the Devfile"
[main 0718673] Add comments to the Devfile
 1 file changed, 3 insertions(+)

Now run odo add binding (either interactively or not), e.g.:

❯ odo add binding --service redis-standalone.Redis.redis.redis.opstreelabs.in --name my-sample-go-redis-standalone
 ✓  Successfully added the binding to the devfile.
Run `odo dev` to create it on the cluster.

Actual behavior

The Devfile has been updated, as expected, to include the ServiceBinding Kubernetes Component. But all YAML comments in it have been surprisingly removed at the same time:

diff --git a/devfile.yaml b/devfile.yaml
index af9a6cd..3112651 100644
--- a/devfile.yaml
+++ b/devfile.yaml
@@ -1,32 +1,58 @@
-# User comments in the Devfile will get removed by running 'odo add binding'
 commands:
 - exec:
-    # Description of the build command
     commandLine: GOCACHE=${PROJECT_SOURCE}/.cache go build main.go
     component: runtime
     group:
       isDefault: true
       kind: build
+    hotReloadCapable: false
     workingDir: ${PROJECT_SOURCE}
   id: build
 - exec:
-    # Description of the run command
     commandLine: ./main
     component: runtime
     group:
       isDefault: true
       kind: run
+    hotReloadCapable: false
     workingDir: ${PROJECT_SOURCE}
   id: run
 components:
 - container:
+    dedicatedPod: false
     endpoints:
     - name: http
+      secure: false
       targetPort: 8080
     image: golang:latest
     memoryLimit: 1024Mi
     mountSources: true
   name: runtime
+- kubernetes:
+    inlined: |
+      apiVersion: binding.operators.coreos.com/v1alpha1
+      kind: ServiceBinding
+      metadata:
+        creationTimestamp: null
+        name: my-sample-go-redis-standalone
+      spec:
+        application:
+          group: apps
+          name: my-sample-go-app
+          resource: deployments
+          version: v1
+        bindAsFiles: true
+        detectBindingResources: true
+        services:
+        - group: redis.redis.opstreelabs.in
+          id: my-sample-go-redis-standalone
+          kind: Redis
+          name: redis-standalone
+          resource: redis
+          version: v1beta1
+      status:
+        secret: ""
+  name: my-sample-go-redis-standalone
 metadata:
   description: Stack with the latest Go version
   displayName: Go Runtime

Expected behavior

Users might have spent some time adding comments and documentation to their Devfiles, which might be quite complex. So existing comments should not be removed, especially since we want the Devfile to be the main source of truth.

I guess we will have the same issue with any other commands that edit the Devfile.

[rm3l]

TL;DR
This might need to remain as a known issue for now 😕

Summary of my quick investigations
Devfile marshaling and unmarshaling are currently not handled directly by odo, but done via the Devfile library, which uses sigs.k8s.io/yaml, which in turn relies on JSON to convert YAML to/from Go structs (like in parse.go).
The rationale behind this is explained in detail in this blog post.
In a nutshell, this allows to reuse JSON tags and any custom MarshalJSON and UnmarshalJSON methods, thus allowing consistent handling of both YAML and JSON (YAML being a superset of JSON).
Since JSON is data-only and does not have any such notion of comments, all comments in a YAML document are lost when converting it into JSON (via yaml.YAMLToJSON).

sigs.k8s.io/yaml is a fork of ghodss/yaml, which is a wrapper around go-yaml (v2). go-yaml v3 brings in comment handling, via a new Node representation that allows preserving comments near the data they describe.
So the recommended way to have comments preserved is via this Node representation, but I guess this will not allow consistent handling of both YAML and JSON.

Found the following open issues and discussions related to these:

• Commands editing the devfile (like odo add binding or odo remove binding) reorder it #5119 and Devfile library used by odo rewrites properties in alphabetical order upon save/write rather than preserving original order upon read. devfile/api#371 (comment) (different effect, on ordering, but seems related to this as it mentions why the Devfile library chose to use sigs.k8s.io/yaml and JSON encoding)
• Should this library move to yaml.v3 kubernetes-sigs/yaml#18, but I don't think this will change anything, as the point behind sigs.k8s.io/yaml is to purposely reuse JSON tags.
• https://stackoverflow.com/questions/55674853/modify-existing-yaml-file-and-add-new-data-and-comments

While it certainly makes sense for the Devfile library to use sigs.k8s.io/yaml (to effectively reuse the JSON tags defined in the Devfile API objects or {Un,}MarshalJSON methods, like DevWorkspaceTemplateSpecContent and Attributes structs), if we want to preserve comments on the odo side, we might not need JSON as an intermediate pivot format, but maybe we could leverage gopkg.in/yaml.v3 directly. This would require more effort on our side.
But I am not sure it actually makes sense to handle this on our side. This should certainly be an issue on the Devfile library side.

[rm3l]

Added this to the Devfile community call (just to open the discussion), but we didn't have enough time to start discussing it. We will discuss it at a later time.

[rm3l]

/status blocked

Issue created on the Devfile library side: devfile/api#919

[github-actions]

A friendly reminder that this issue had no activity for 90 days. Stale issues will be closed after an additional 30 days of inactivity.