build/manifest: print whole manifest variable as it is

[ilgooz]

Summary

Print whole manifest variable as it is while generating manifest files for both JS and Go sources.

• added new vargen package to do code generation for Go's runtime variables.

• notice that the code generation package (github.com/dave/jennifer/jen) used in vargen package has two cons:

  • it reorders struct types' fields by alphabet instead of leaving it as it appears during struct's initialization.
  • it does not put new lines for evert struct field if struct is initialized with under 2 fields. this might be annoying for some.

• we could fix cons described above by creating our own code generator that directly builds code by bare writing to a buffer or by using go/ast but both ways will add more code to maintain. and this might not make sense for some small advantage.

• notice that generated Go variable may contain slice, array, map and struct values with no members. this is valid because they must have been initialized with no members. zero values on the other hand are discarded and won't appear in the generated code.

• notice that generated JS variable may contain zero values. this is valid because code generation for JS made by using JSON marshaling and encoding/json removes empty values only when omitempty tag is used.

• this commit introduces breaking changes since generated manifest.js and manifest.go files are now exporting model.Manifest type.

• ran go mod tidy.

Related Ticket

fixes #72.

[ilgooz]

It seems that CI cannot make tests pass where it's all OK at my local machine. I'll check and try to fix CI errors.

[lieut-data]

Leaving the full review to @iomodo and @mickmister, but I wonder if there's another, third option that might be simpler: use ManifestFromJson and just write out the raw JSON as an inline parameter to same? One downside is the need for the plugin to have to "parse" the JSON on each start, but then we wouldn't need any extra tools to process it.

[ilgooz]

Leaving the full review to @iomodo and @mickmister, but I wonder if there's another, third option that might be simpler: use ManifestFromJson and just write out the raw JSON as an inline parameter to same? One downside is the need for the plugin to have to "parse" the JSON on each start, but then we wouldn't need any extra tools to process it.

Totally right. I might be overcomplicated things with vargen. The only pros it provides is, it just renders a usual Go code which looks better for the eyes.

On the other hand a solution by parsing JSON encoded manifest to model.Manifest var is the easiest and less error prone for development and maintenance work. We can do the decoding within init() func and it'll get cached to a var for the program's runtime.

It is also possible to print this JSON data in a human friendly way so developers still can look at the manifest.go to get an overall idea about how it looks like.

[lieut-data]

It is also possible to print this JSON data in a human friendly way so developers still can look at the manifest.go to get an overall idea about how it looks like.

Agreed, let's definitely render this in a human readable way. I do like the simplicity of the rendered Go code provided by vargen, but it does seem to require a lot of extra overhead. I had somewhat expected it (or some similar package) to be able to write out an arbitrary struct without further configuration: that would be the ideal solution.

[ilgooz]

@lieut-data I'll do the updates to use JSON approach over vargen for the Go version once we have a full agreement!

Hey @prapti @iomodo @mickmister what are your thoughts about this?

[ilgooz]

I had somewhat expected it (or some similar package) to be able to write out an arbitrary struct without further configuration: that would be the ideal solution.

Before coming up with vargen, I researched for a tool to do this for us, to write Go structs from runtime values and pretty is the closet one to what we would look for.

But even it has a few cons, it does not produce a valid Go code for empty maps and it puts some extra type indicator for values that we usually don't have in a regular Go code. I didn't prefer to create a PR to fix these because the project seems inactive now.

[iomodo]

Great PR @ilgooz! Go code does look nice, but I'd still vote for a JSON version - a much, much simpler code.

[ilgooz]

ready to review.

switched to JSON unmarshaling by 8db2d85098ad877f4a253cd97b91f2d67834d798.

[ilgooz]

Not blocking on the PR, but I'm wondering why we parse the manifest on compile time. We already include it into the bundle and could parse the JSON on run time. This would remove the need of a auto generated server/manifest.go which would resolve the pain point that server/manifest.go changes on every release. At least @levb would love this 😉

I also find this a better and flexible approach but I think we should be providing these kind of functionalities with language SDKs.

For ex:

1- go: plugin.Manifest() *model.Manifest > makes a lookup to find manifest.json in plugin's root, parses, caches and returns manifest.

2- js: plugin.Manifest(): Manifest > same as the Go version. I don't know if we have but we need a public MM npm package (js SDK) to enable this.

[lieut-data]

@hanzei / @ilgooz, on the subject of "why", this idea originated from https://github.com/mattermost/mattermost-plugin-demo/pull/74#discussion_r344851228. One cannot resolve one's own bundle without first making an RPC call (or speculating where on disk you might find it). This seems counterintuitive, hence just making it explicit.

@ilgooz, thanks for updating the spacing! I don't see the actual generated files having been updated yet, though, and I note that our linter seems to be complaining about wanting 4 spaces, plus wanting single quotes, etc. Perhaps we converge on 4, regenerate, and split apart the Go-generated code from the JS-generated code to achieve linting satisfaction?

[levb]

... We already include it into the bundle and could parse the JSON on run time. This would remove the need of a auto generated server/manifest.go which would resolve the pain point that server/manifest.go changes on every release. At least @levb would love this 😉

(Sorry I am not in the context of this PR, so ignore if irrelevant) I always thought that the manifest files were a redundant hack - would love to see them disappear. JS could include plugin.json natively, and in golang it can be pre-set in p.MattermostPlugin by the time the plugin is loaded.

[ilgooz]

@ilgooz, thanks for updating the spacing! I don't see the actual generated files having been updated yet, though, and I note that our linter seems to be complaining about wanting 4 spaces, plus wanting single quotes, etc. Perhaps we converge on 4, regenerate, and split apart the Go-generated code from the JS-generated code to achieve linting satisfaction?

I was holding my update to see if I can come up with an elegant solution but seems like running multiple regexes are the only option that I can find for now.

Pushing the changes with 147412b to not block this PR.

[lieut-data]

@levb, agree that we might find a way to supply the manifest entirely at runtime. For now, this sticks with the current approach of building into the binary, but supplies the whole manifest instead of just a few fields, avoiding the need to lookup the manifest in related API calls.

[mattermod]

This issue has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

/cc @jasonblais @hanzei

[jasonblais]

@prapti Quick reminder to help with this PR testing when you have time :)