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

Home Assistant Add-on #20

Merged
merged 6 commits into from Nov 9, 2023
Merged

Home Assistant Add-on #20

merged 6 commits into from Nov 9, 2023

Conversation

dominikandreas
Copy link
Collaborator

This PR adds home assistant add-on support. It is functional, but still a work in progress.

Adresses #17

Todo:

  • update URLs in repository.json and config.yaml
  • publish data with home assistant discovery support (separate PR for this?)
  • get mqtt user, pw via bashio
  • ? (Happy to accept feedback)

@DennisOSRM
Copy link
Owner

The pull request looks great. Is there a chance we can add a test or two to verify we don't break it by accident with future changes?

@dominikandreas
Copy link
Collaborator Author

Do you have anything in particular in mind? I'm not sure how to do automatic testing with home assistant add-ons. As long as the rust application runs in the docker image which is referenced in the Dockerfile of the addon, the addon should work as well (given non breaking changes in the CLI). Also, the addon Dockerfile references a specific version of the docker image from this repo, so even with breaking changes in the rust code, the addon keeps working until we manually upgrade it.

I suggest whoever makes changes to the add-on does some manual testing first to ensure everything works as expected. Maybe long term we can also come up with some means of automatic testing. For this we would need:

  • a docker service emulating a HMS
  • a docker service running an mqtt server
  • a docker service running some tests and validates mqtt messages against some predefined schema
    Such a test framework would of course also be useful for testing the rest of the codebase

@DennisOSRM
Copy link
Owner

That makes sense. I'll add an issue into the backlog that we add testing. In the meantime we can move ahead here and merge the PR. Once again, great work. Thanks so much.

@DennisOSRM DennisOSRM merged commit 5d3d8f9 into DennisOSRM:main Nov 9, 2023
1 check passed
@dominikandreas
Copy link
Collaborator Author

As mentioned, there are still some other todos. But I can also address them in a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants