Add custom JSON generation

[chrisst]

to_json will print out a more human readable representation of the api
representation. It uses api property names as json key's in order to provide
more useful breadcrumbs and viewing when collapsed.

Example output:

---

[all]

no-op

[terraform]

[terraform-beta]

[ansible]

[inspec]

[rileykarson]

Quick high level question: What's the use case for this change? Debugging? It isn't clear to me as a reader, and it just looks like we're adding unused code.

If this is code that's going to sit around at HEAD and only be used at debugging time, we should say so above the method in a comment and have tests to make sure we don't break it with changes like variable name changes.

Edit: also as @rambleraptor points out, what's the big win over .to_yaml?

[modular-magician]

I am a robot that works on MagicModules PRs!
I checked the downstream repositories (see README.md for which ones I can write to), and none of them seem to have any changes.

Once this PR is approved, you can feel free to merge it without taking any further steps.

[rambleraptor]

I'm gonna be a pain. Apologies for this in advance.

First, why JSON over YAML? By outputting to YAML, we gain the ability to get output YAML and then feed it immediately back into MM if necessary. We can't do that in JSON.

My biggest concern is the .to_s method. We don't call .to_json anywhere (yet?). I'm scared about overriding .to_s and then it ends up making debugging harder down-the-line because .to_s is used in a million places throughout the Ruby compiler.

Small aside: For deserialization, we will have to build some kind of .to_<yaml, json, s> methods (as you're doing now). Psych has similar things for dealing with YAML (where you get to control serialization and deserialization). If we're going to do all of these methods to handle deserialization, I think we should consider doing the same for serialization too.

TL;DR why JSON and changing .to_s scares me.

[rambleraptor]

See note.

[chrisst]

@rambleraptor -
why json - specifically because we use YAML everywhere so overriding the yaml serialization is much more likely to affect actual MM functionality. If I was to use yaml I'd likely be writing the entire serializer instead of being about to shim on top of existing methods such as to_json. I don't really see a deserializer useful, because as you pointed out: we should use yaml if we plan on programmatically consuming this :)

to_s - In my experience overriding a class's to string method is pretty standard behavior and should only affect debug output. Java best practices usually mandate this. The current default to_s just outputs the class name and the memory address of the instance, eg: #<Provider::Terraform::CustomCode:0x00007f87bcc4e370> . If we have code that relies on the string serialization of a class we need to stop it, as it would be pretty bad code smell. Ruby should always be using duck typing or is_a for type checking.

[chrisst]

@rileykarson I was planning on adding command line options that could output this json to a file or stdout. There's already the ability to output the raw yaml, but it's pretty worthless for human consumption. However I was thinking given how overloaded the compiler.rb is right now I'll chat IRL with people about design before shimming it in.

As for tests, I can definitely add something.

[rileykarson]

I'm still confused what the use case for outputting all this would be from the compiler- my strawman here is that dumping all of Compute [much less all of our resources] is a useless amount of data for human consumption, whether it's as JSON/yaml/etc.

If we're looking for more human readable output, I'd want to look for a way to remove some fields / nested objects (as in nested ruby objects, not NestedObject 🙂) from our YAML instead unless we have a very strong reason to use JSON instead.

Want to talk offline/over chat about what you want this feature for?

[chrisst]

The background is that I'm trying to refactor the Disk properties (which are a hot mess right now) and I need a way to determine the output of my changes to a particular resource. I can output compute to a json file, apply refactors and then compare the new json output against the previously generated one. Diff tools allow me to see the difference in the two in a way that the current yaml output doesn't make easy. Happy to chat more if this doesn't make full sense.

[rambleraptor]

I'd like to get @SirGitsalot's opinion on this. I have a weak opinion against this, but it's easily changed.

[rileykarson]

Discussed over chat, but to restate my opinion here: If we need this to debug our YAML definitions, the real problem seems like we made the YAML definitions too complex. Disk/RegionDisk having 3!! inlined files (disk_parameters.yaml, disk_type_properties.yaml, and disks.yaml) is the real issue, and I'm for inlining all of it.

[chrisst]

I think you've conflated the problem this PR is trying to solve. The question here is "should we add a custom json serializer" not "how to best solve upstream complexity". You can still inline everything upstream with or without this addition. I added some reasons for the PR (see image of nested output) but I any tool that integrates with well formatted JSON can consume the output.

For instance using JQ you can answer "what are all the resources supporting a kmskeyname property" using json searching syntax:

cat file.json | jq '[paths] | .[] | select (contains(["kmsKeyName"]))'

[rambleraptor]

I agree that we've drifted from the point. And I do like being able to do jq magicks on api.yaml

My opinions are all weak, so I'm passing over to @SirGitsalot for a final verdict.

[rileykarson]

Let's talk about this in the MM sync this week!

While you're correct that this PR asks the question "should we add a custom json serializer", it doesn't (well, didn't) include context on why we're asking that question. The thing is, I'm not sure that the problem isn't "our yaml is too complex". So let's talk about the reasons we would want this feature in a higher bandwidth setting 🙂

[modular-magician]

I am a robot that works on MagicModules PRs!
I checked the downstream repositories (see README.md for which ones I can write to), and none of them seem to have any changes.

Once this PR is approved, you can feel free to merge it without taking any further steps.

[rileykarson]

Github won't let me remove the big ol' red mark w/o a comment.

outdated

[chrisst]

Talking with the team today the consensus was in favor of overriding to_s and to_json to provide an easier output when using pry or trying to serialize complex objects.

[chrisst]

@rambleraptor as per conversation yesterday and in the MM meeting - the default to_s is now pretty json. This PR is ready for review

[rambleraptor]

I'm fine with this!

Can you make a comment somewhere that JSON.pretty_generate will call the .to_json method?

[modular-magician]

I am a robot that works on MagicModules PRs!

I built this PR into one or more PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#404
depends: hashicorp/terraform-provider-google#2979

[modular-magician]

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 2dca01c) have been included in your existing downstream PRs.

[modular-magician]

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 7d468d9) have been included in your existing downstream PRs.

[modular-magician]

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit 13bb941) have been included in your existing downstream PRs.

[modular-magician]

I am (still) a robot that works on MagicModules PRs!

I just wanted to let you know that your changes (as of commit a3cdb95) have been included in your existing downstream PRs.