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

🐛 Implement sorting on structs when marshaling #431

Merged
merged 4 commits into from
Nov 17, 2023

Conversation

JonahSussman
Copy link
Contributor

Fixes #418

demo-output.yaml's diff looks really ugly, but all this PR does is shuffle some stuff around.

With this, we may not the sed and yq stuff in this workflow anymore:

- name: run demo image and ensure violations output unchanged
run: |
podman run -v $(pwd)/demo-output.yaml:/analyzer-lsp/output.yaml:Z localhost/testing:latest
diff \
<(sort <(sed 's/incidents\.[0-9]\+/incidents\.x/g' <(yq -P 'sort_keys(..)' -o=props <(git show HEAD:demo-output.yaml)))) \
<(sort <(sed 's/incidents\.[0-9]\+/incidents\.x/g' <(yq -P 'sort_keys(..)' -o=props <(cat demo-output.yaml))))

@JonahSussman JonahSussman marked this pull request as ready for review November 15, 2023 21:55
Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just thinking out loud here, but what do we think about doing this for JSON while we do this?

I don't think encoding/json does sorting by default so it may be worth it.

output/v1/konveyor/violations.go Outdated Show resolved Hide resolved
return d, nil
}

func (d Dep) cmpLess(other Dep) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this used for?

Copy link
Contributor Author

@JonahSussman JonahSussman Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I accidentally left it in before committing. I was doing some exploration on both JSON and YAML. Hopefully the "Implemented JSON support" commit elucidates it

Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
@JonahSussman JonahSussman force-pushed the 418 branch 7 times, most recently from 8616c58 to 275f0f2 Compare November 16, 2023 16:42
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Copy link
Contributor

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] marshalling of output is non-deterministic
2 participants