search

billimek / billimek-charts

DEPRECATED - new home is https://github.com/k8s-at-home/charts
Apache License 2.0
89 stars 52 forks source link

[zigbee2mqtt] ui application #296

Closed masantiago closed 4 years ago

masantiago commented 4 years ago

Special notes for your reviewer:

Adding UI application to zigbee2mqtt so that both zigbee and zwave are identical in functionality

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

billimek commented 4 years ago

Should the new probes, service, and ingress sections of the values.yaml file be encapsulated within the ui section? If UI is not enabled, it doesn't seem as if they will have any use?

masantiago commented 4 years ago

It would be helpful that @ishioni can also have a look to the PR. I like his idea of a generic Configmap for the ui settings in PR-294, although not repeating the parameters already available in the config section. That's why I eventually fixed them to prevent collision. BTW, they could be interpreted as overrides.

As said, hopefully we can make the best of two PRs before any merge.

ishioni commented 4 years ago

@masantiago

I like his idea of a generic Configmap for the ui settings in PR-294, although not repeating the parameters already available in the config section

You can have both. Just calculate the mqtt endpoint like you already do, set it in the configmap, then accept any other option from the config section for z2ma, and don't specify that option at all in values.yaml. You can add a comment that it's already set from z2m. If the user decides for some reason to add it anyway, it's on them, and it will work anyway, since helm doesn't deduplicate, assuming you'll loop over the options after calculating the mqtt endpoint, the user-set option will win over in the container.

Oh, and if there's anything i'd like to change in this, is that in my PR i really tried to retain the naming of z2ma. assistant: instead of ui: looks much better, ditto for the resource names.

masantiago commented 4 years ago

@ishioni agreed on all your comments and updated the code accordingly. In the latest version, you will find that I have also moved the probes, ingress and service within the assistant section. Nevertheless, only a commit revert is needed to come back to the original situation (at root level). I would be happy with any of those :smile: @billimek , please advise which option you prefer.