[MM-23184] Omit empty labels list

[hanzei]

Summary

This make the json output a bit nicer. Forgot about the case in #42

Ticket Link

https://mattermost.atlassian.net/browse/MM-23184

[hanzei]

@lieut-data @ali-farooq0 I would like to merge master into production soon and also include this change. Would you give this please a quick review?

[alifarooq0]

@hanzei what's the reason for this change?

I kinda like the field labels to always be there, so it's clear what fields to expect. What if we marshalled it as label: [] instead?

[lieut-data]

Agree with @ali-farooq0's question -- uncertain as to the motivation of this change. Agree it would be cleaner, but at this point, is there something else that makes it high-impact? We'd also want to carefully test this with the Mattermost server to avoid any unexpected surprises.

[hanzei]

@ali-farooq0 @lieut-data I should have given more background to my changes:

When you are currently running make plugis.json on master, the diff contains a lot of "labels": null, which get added. This is caused by me removing the labels field manually in #42, but forgetting that an nil list is marshaled as null.

There are three ways to sync the output of make plugis.json with the committed plugis.json:

1. Omit labels (This PR)
2. Run make plugis.json and commit the result
3. Change the generator to have an empty, but not nil list of labels by default , then run plugins.json and commit the result and code changes

I'm mostly 0/5 on which way to go. I did pick 1. because it's the smallest diff and to me it seams like the cleanest approach.

I did test my changes with the latest webapp and server.

Please let me know what you think about this and which way you prefer.

[lieut-data]

Got it! I also forgot that since the marketplace consumes plugin.json, it probably ends up at the same internal representation relative to what the Mattermost server sees, so less of a concern.

Thanks for clarifying, @hanzei :)

[alifarooq0]

Thanks @hanzei. I guess this would be more of a concern if we had other open clients using this marketplace directly, but since we don't this might be OK for now.

I personally don't mind having null label values for plugins.json or doing something special for our internal plugins.json generation, I am more worried about the API responses that the marketplace returns now, as in some cases it'll have the label field and in others it wont.

Do you think the consistency issue is worth addressing in the long run? Perhaps as a HW ticket. Thoughts?

[hanzei]

I personally don't mind having null label values for plugins.json or doing something special for our internal plugins.json generation, I am more worried about the API responses that the marketplace returns now, as in some cases it'll have the label field and in others it wont.

Do you think the consistency issue is worth addressing in the long run? Perhaps as a HW ticket. Thoughts?

@ali-farooq0 The same struct is used to generate plugins.json that is used for the API responses. Could you elaborate a bit how you would not omit the field for the API responses, but would for plugins.json without duplicating code?

[hanzei]

@ali-farooq0 Heads up that I'm going merge the PR as it is and we can iterate if needed.

[alifarooq0]

@ali-farooq0 The same struct is used to generate plugins.json that is used for the API responses. Could you elaborate a bit how you would not omit the field for the API responses, but would for plugins.json without duplicating code?

@hanzei what's the issue with having nulls in the generated plugins.json?

[hanzei]

@ali-farooq0 There is none. Having null inside of plugins.json was one of the considered options in #48 (comment)

[alifarooq0]

Ah gotcha. Yeaa i missed that 🤦‍♂. Would've preferred that over the omit :) but oh well. Since we don't have an external client relying on this, not the end of the world 👍