Fix marshalling scalar blocks

[K-Phoen]

Fixes #245

There is currently a bug in go-yaml/yaml causing indentation hints on scalar blocks to be set incorrectly. As a result, they can not be unmarshalled.

A PR has been open upstream a few years ago but hasn't seen any meaningful activity recently.

This PR is an attempt at fixing this (un)marshalling issue by using an alternative, regurlarly maintained, YAML library: goccy/go-yaml

Some quick manual testing showed that goccy/go-yaml resolves #245, but there is more validation needed to decide if switching to a new dependency is viable:

• goccy/go-yaml is highly likely to marshal resources differently. While some degree of change is probably acceptable, I currently don't know how different marshalled resources are.
• there isn't any unit test covering the issue, I will add one.

[CLAassistant]

All committers have signed the CLA.

[K-Phoen]

The marshaled resources are actually fairly similar with the new library. The main differences come from inconsistencies in go-yaml/yaml.

For example, it would output:

    panels:
        - id: 4
          gridPos:
            h: 22
            w: 10
            x: 14
            "y": 0
          tags:
            - hosted-metrics

(note the inconsistent indentation: mostly 4 spaces but sometimes 2 spaces)

While goccy/go-yaml generates:

    panels:
        - id: 4
          gridPos:
              h: 22
              w: 10
              x: 14
              "y": 0
          tags:
              - hosted-metrics

(consistent, 4 spaces indentation)

[joanlopez]

Hi @K-Phoen,

I tried to grr pull and grr diff resources (i.e. dashboards) from a Grafana instance, and while I'd expect no differences, I see a few of them, that look like:

Dashboard.hOipiybnz changes detected:
--- Remote
+++ Local
@@ -292,7 +292,7 @@
                 language: plaintext
                 showLineNumbers: false
                 showMiniMap: false
-            content: |4-
+            content: |-
                 <h1 style="text-align: center;">Docker</h1>
                 <h3 style="text-align: center;"><a href="/d/MQ5mdQbnk/docker-monitoring">More</a></h1>
             mode: html

It also looks like similar differences are detected when following the same steps with the current version (using go-yaml/yaml). So, do you think these are acceptable? Or should these completely disappear with the lib replacement?

--

Other than that, is there any other test that you would suggest doing before merging this PR? (cc/ @malcolmholmes)
I'd be happy to help with getting this merged. So far, I've merged latest changes from master to solve conflicts.

Thanks!

[malcolmholmes]

The thing that stopped me merging this PR is the fact that it results in whitespace (and other) no-op diffs. Any users that are using diff to detect meaningful changes would find a whitespace change annoying.

And given that there's a simple solution (don't lead your queries with a newline), I didn't tackle the question of whether this was a valid/valuable change.

I'm totally open to using a different approach to YAML marshalling, I just want to know its enough to justify any discomfort users may experience.

[joanlopez]

The thing that stopped me merging this PR is the fact that it results in whitespace (and other) no-op diffs. Any users that are using diff to detect meaningful changes would find a whitespace change annoying.

And given that there's a simple solution (don't lead your queries with a newline), I didn't tackle the question of whether this was a valid/valuable change.

I'm totally open to using a different approach to YAML marshalling, I just want to know its enough to justify any discomfort users may experience.

What do you think would be actually annoying?

• grr pull + git status would result in a significant amount of changes
• grr diff would result in a significant amount of changes
• both

--

In my opinion (which might be totally wrong!), what I think that could be annoying is only the 2nd, cause would be equivalent to saying "your local files aren't in sync" - while this would not necessarily be true.

In that sense, I think we could kinda sort it with a different diff algorithm, that compares "objects" (like JSON comparison) not string literals. That should reduce most (if not all) the no-op diffs (it would still display white spaces and newlines within values, but I guess that should be fair cause they could technically have different meanings / end in different results.

Regarding the 1st one, I guess it's like when adding a new linter rule or something like that, could be a bit annoying as well, but less likely to cause problems if we provide a real comparator (grr diff tells the "truth").

Wdyt? cc/ @K-Phoen

[malcolmholmes]

Interesting questions - you're totally on the mark here. A JSON diff would be amazing, actually. And whitespace changes are less of a bad thing in PRs nowadays due to clever diff highlighting whitespace differently.

[malcolmholmes]

FTR: This PR improves matters but does not resolve them. Grafana dashboards can do some weird things with their JSON. Here's some issues that show up with this library when parsing and rendering dashboards found in the wild: goccy/go-yaml#418

[malcolmholmes]

converted to draft as there's nothing final here

[malcolmholmes]

Unfortunately, this library introduces a separate set of issues caused by the new library. I'm closing it for now. Should this library resolve its issues, we can re-open/reconsider. Keeping #245 open to keep track of the original issue.