Auto-generate source code from json schema defintions and go generate instructions

[petemoore]

@jonasfj @gregarndt

Work in progress to auto-generate code based on json schema for engine config, plugin config, and plugin payload schema config...

Raising PR before work is done so interested parties can see what I am playing with. I'll add a comment when it is ready for formal review and is complete - it is just my work in progress at the moment.

[jonasfj]

I would propose the following:

1. this tool be called go-composite-schema
2. Commandline be on the form: go-composite-schema [--required] <property> <input.yml> <output.go>
3. <input.yml> must be a JSON schema
4. <output.go> will be a file as follows:

type <schema-title> struct {
  .... // Whatever is generated from the schema
};

var <schema-title>Schema = func() runtime.CompositeSchema {
  schema, err := NewCompositeSchema('<property>', `
    <contents of input.yml as json>
  `, <true if --required>, func() interface{} { return &<schema-title>{}; });
  if err != nil {
    panic(err)
  }
  return schema;
}()

Maybe instead of using <schema-title> we could define a property called typeName on the schema, which jsonschema2go could use if present. That way we can name the type something sane, without compromising our documentation.

@petemoore, I would even be open to allowing you to add typeName as a top-level property to all our schemas in all TC components... Granted that would obviously be a lot of work to do :)

With this tool, the PluginProvider implementation becomes very simple, you just return <schema-title>Schema from the ConfigSchema() method...
I don't advice that we auto generate that method, because it's very easy to write... And this way we have a generation tool that ensures all of our CompositeSchemas are sane... And we can use it where ever we have CompositeSchemas. Whather for engine/plugin payload, or engine/plugins config, or config for some completely different sub-system we make in the future.

---

On-topic, I suggest that we focus our energy on something other than this... but if you want to finish it up, this is a direction that would keep it simple, relevant and ensure that it can be widely used throughout our codebase. By focusing on CompositeSchema rather than plugins and engines.

Note: main value here is obviously that we can write in YAML, we can validate the schema again the meta-schema upfront, and we probably should do that :)

[petemoore]

@jonasfj I think this is in a state you can review it in now. Note, I added some dummy config files for illustrative purposes - we can remove them if you like.

[petemoore]

Note, I wanted to create a yml file for the global config (everything except plugins and engines) but haven't yet written anything to use it. Probably we could use go-composite-schema for this too.

[jonasfj]

I was tempted to do:
var configSchema = func() runtime.CompositeSchema { .... return ... ; }()

So it's just a variable...

Also quick question, is it possible to make these types private... I don't want to export them from the package.
Obviously RootVolume need to be exported for json to work... but Config struct { does not.
Similar for ConfigSchema() no need to export it... Also I suspect it's easier and safer to do it as a variable..
That way if there is a panic() inside the function it'll always happen when the program starts, so tests would never work, if the is a panic()...

[petemoore]

Are you sure we don't want to export them - I think it wouldn't hurt if someone using a package could see the json schema, as this helps when we write automated tooling to e.g. publish schemas etc. Of course, if we really don't want it, we can - it feels though correct that we should be able to generically infer stuff about plugins/engines etc. That said, the generated code is not for the engine/plugin itself, but just package identifiers, so yeah, I guess we don't want to export that. Fair enough. Yes, I can add support for that.

[petemoore]

We can stick the function as a variable, and admittedly that means it would be easier to mock (although I'm not sure why we'd mock it - but at least we'd be able to). However, I don't think it means we'll have startup-panics, as the function would be defined, but not executed, wouldn't it? Or would something call that function at startup?

[jonasfj]

Or would something call that function at startup?

The pattern I wrote here:
var configSchema = func() runtime.CompositeSchema { .... return ... ; }()
Notice the subtle }() that the end... I'm actually setting configSchema to be the result of an anonymous function that is immediately invoked :)

So it'll be executed before main() is called.

[jonasfj]

Are you sure we don't want to export them - I think it wouldn't hurt if someone using a package could see the json schema, as this helps when we write automated tooling to e.g. publish schemas etc. Of course, if we really don't want it, we can - it feels though correct that we should be able to generically infer stuff about plugins/engines etc. That said, the generated code is not for the engine/plugin itself, but just package identifiers, so yeah, I guess we don't want to export that. Fair enough. Yes, I can add support for that.

This reads like one of my comments where I start out disagreeing and end up agreeing as I write more arguments :) he he...
(honestly I do those kind of comments too often).

In short, yes, if someone is using the package, they should use it by importing it and then use extpoints to find the EngineProvider/PluginProvider of it.. which will return the schema in question...

[petemoore]

Notice the subtle }() that the end...

D'oh! Yeah, totally makes sense. This also has the nice side effect that init() functions inside plugin/engine packages that register plugin and engine providers can even safely refer to the generated variables (since variables are initialised before init() is called). 👍

[jonasfj]

Generally I really like this...

[petemoore]

@jonasfj can we move go-import-subtree into taskcluster org?

[jonasfj]

sure... but we'll need to update references too... can we do it separately...

Note: added you can garndt as collaborators for now if you have any hacks for it...
I'm very much trying to keep go-import-subtree super generic and not taskcluster specific, which I why I wasn't exactly sure where to put it...

[jonasfj]

just merge this and we can work some more on the small things... I would rather have it in than out...

even if we change the names a bit, it's easy to refactor...

[petemoore]

Sorry I got caught up with other issues yesterday. I'm happy to make the fixes before merging - I agree with them all! :)

I actually created the review request for this PR in bugzilla only because I was cleaning up bug-state at the time, not because I was finished - I should have clarified in the bug. I'll ping you when it is done, and then hopefully we can review again then, and all being well, merge.

[gregarndt]

This is something that has been on my mind for awhile regarding how we treat pull requests and patches. I think for taskcluster-worker we should consider a PR as the initial starting point that will be mutable and not the final solution. The goal is that once it's merged it's the final agreed upon solution but not before hand.

This way everyone is under the understanding that when a PR is open, it's not that it's a final review, but rather just a request for feedback in the early stages of the PR changes.

[petemoore]

@jonasfj good enough to go now?

[petemoore]

jonasfj
19:18 pmoore ship it