Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mqtt.homeassistent] Bundle JINJA dependency in footer.xml #17326

Closed
wants to merge 2 commits into from

Conversation

lsiepel
Copy link
Contributor

@lsiepel lsiepel commented Aug 24, 2024

Fixes #6875

The feature file from mqtt.homeassistent is overruled by the footer.xml. Result; this is the only place to add the dependency. The overhead for loading NINJA for the other mqtt bindings is minimal and i would greatly improve the user experience.

Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
@lsiepel lsiepel added the enhancement An enhancement or new feature for an existing add-on label Aug 24, 2024
@lsiepel lsiepel requested a review from a team as a code owner August 24, 2024 21:22
@lsiepel lsiepel requested a review from ccutrer August 24, 2024 21:28
@ccutrer
Copy link
Contributor

ccutrer commented Aug 24, 2024

While I'm all for this, I'd rather bundle Jinjava directly if we're going this route. Several components are missing some functionality (notably, template schema lights) because we need to use a lower level interface than the transformation service can provide.

Last time I attempted to do that, it was recommended I add a new feature to core that both the transformation service and the MQTT binding can depend on. And I never could figure it out.

So.... given that background, and this PR, are we okay with "duplicating" jinjava in this binding in the future?

@jimtng
Copy link
Contributor

jimtng commented Aug 25, 2024

Pardon my ignorance as I haven't actually looked into this. How hard would it be to actually completely remove the requirement for/use of Jinja in mqtt.homeassistant?

@ccutrer
Copy link
Contributor

ccutrer commented Aug 25, 2024

Completely impossible. Jinja is intrinsic to how Home Assistant discovery describes how to access data. There are a few devices that could get by with a simplified barebones implementation, but most not.

@J-N-K
Copy link
Member

J-N-K commented Aug 25, 2024

There is a rule that no add-on is allowed to depend on another add-on. This is the reason why MQTT (or Bluetooth) are one add-on instead of several. If you add a dependency here, this rule will bee violated.

@lsiepel
Copy link
Contributor Author

lsiepel commented Aug 27, 2024

There is a rule that no add-on is allowed to depend on another add-on. This is the reason why MQTT (or Bluetooth) are one add-on instead of several. If you add a dependency here, this rule will bee violated.

It has been a while ago that you commented on this, did you mean something different by adding this to the feature file?

Last time I attempted to do that, it was recommended I add a new feature to core that both the transformation service and the MQTT binding can depend on. And I never could figure it out.

What did you try? Maybe we can try to figure that out with the help from @J-N-K ?

@ccutrer
Copy link
Contributor

ccutrer commented Aug 27, 2024

What did you try? Maybe we can try to figure that out with the help from @J-N-K ?

It was when I was trying to implement Template Schema Lights as part of #13413. I broke it and JSON Schema Lights out. JSON Schema was merged as a separate PR. https://github.com/ccutrer/openhab-addons/tree/mqtt-homeassistant-template-schema-light has my work from then, but I never got to the point of getting the Jinjava bundle installed as a separate feature, so didn't adjust the binding code to use it directly.

@J-N-K
Copy link
Member

J-N-K commented Aug 31, 2024

@lsiepel There is a difference between a technical solution that solves the issue (which is what I suggested in the comment you linked above) and policy. I currently can't find a link but it has been stated several times in the past (e.g. by @kaikreuzer) that there should be no dependency from one add-on to another add-on.

IMO there JinJava dependencies should be OSGi-ified (see openhab-osgiify repository) and then both (mqtt.homeassistant and jinja transformation) should depend on these artifacts.

@jimtng
Copy link
Contributor

jimtng commented Aug 31, 2024

I've opened a PR to put Jinjava back into openhab-osgiified: openhab/openhab-osgiify#48
Also #17356

Jinja transform used to use the osgiified jinjava but it got reversed in 2021 #10578 openhab/openhab-osgiify#24

@wborn wborn added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Sep 1, 2024
@lsiepel
Copy link
Contributor Author

lsiepel commented Sep 1, 2024

This PR is about to be replaced by a new PR, see previous comments

@lsiepel lsiepel closed this Sep 1, 2024
@lsiepel lsiepel deleted the ha-bundle-jinja branch September 1, 2024 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mqtt.homeassistant] JINJA transformation should be installed when MQTT binding is installed
5 participants