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

Improve error handling, Nightly build #44

Merged
merged 29 commits into from
Dec 10, 2023

Conversation

dominikandreas
Copy link
Collaborator

  • run cargo clippy and fix all issues
  • run cargo fmt to format all source files

This PR includes the following updates:

  • add a --debug_logging parameter and corresponding option in Home Assistant
  • use try_publish instead of publish to propagate MQTT connection errors
  • add home assistant nightly addon
  • update addon versions

It should've probably been separate PRs, but I haven't had the time yet to separate the work into multiple branches. I can do that over the next couple of days if necessary, but wanted to open the PR already to get some feedback.

src/logging.rs Outdated
@@ -4,7 +4,13 @@ use chrono::Local;
use env_logger::Builder;
use log::LevelFilter;

pub fn init_logger() {
pub fn init_logger(debug: bool) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible to do this from an environment variable without a code change:

RUST_LOG=debug

Copy link
Owner

@DennisOSRM DennisOSRM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR looks great! I think we could get it in without too many changes other than removing the Rust code changes and using the environment variable to trigger verbose logging.

@DennisOSRM
Copy link
Owner

Nice! I guess the build/push to docker hub will 'just work'™️ once the PR is merged, or does it require additional changes in configuration?

@dominikandreas
Copy link
Collaborator Author

dominikandreas commented Nov 30, 2023

For the nightly build you still need to define the DOCKER_HUB_REPOSITORY_NIGHTLY secret in the repository settings. I think then it should work after the merge.

Still need to

  • do some final testing tomorrow once the is up
  • update the addon dockerfiles with references to

@DennisOSRM
Copy link
Owner

Ah, right. I'll put that secret in tomorrow.

@dominikandreas
Copy link
Collaborator Author

thx for the collaboration invite :-) everything seems to be working in the addon so far, but I'll test again tomorrow when the sun is up

@dominikandreas
Copy link
Collaborator Author

Screenshot_20231201-125927
I am getting updates from the inverter in home assistant, but there are still messages about failures to publish.
Also the addon seems to have reported the same values over a couple of hours today until I restarted it. I think we have to decrease the frequency of queries to the inverter

@DennisOSRM
Copy link
Owner

Did you try with the latest changes from yesterday? It will try to deliver three times before it gives up. Also, requesting data more often than every 30.5 seconds will prohibit the inverter from updating. Then the data stays the same.

@DennisOSRM
Copy link
Owner

For the nightly build you still need to define the DOCKER_HUB_REPOSITORY_NIGHTLY secret in the repository settings. I think then it should work after the merge.

Still need to

Added DOCKER_HUB_REPOSITORY_NIGHTLY to point to https://hub.docker.com/repository/docker/dennisosrm/hms-mqtt-publisher-nightly/general

@dominikandreas
Copy link
Collaborator Author

Haven't been able to spend any time on this, I'll probably do some more testing tomorrow

@DennisOSRM
Copy link
Owner

Sounds good. Thanks for the heads up.

@dominikandreas
Copy link
Collaborator Author

Looks good now! I've updated the nightly addon as well as the main one (after testing the nightly version) to the latest build from main.

I've also fixed the workflow files, the builds were failing because it couldn't access the GIT_HASH. I think the nightly workflow is still failing because it can't access the secrets before it's merged.

I think the PR should be ready to be merged now.

@DennisOSRM
Copy link
Owner

Cool. Then let's get it in!


The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).

## [v0.4]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't released 0.3 yet. 😀 I can fix the version number as part of the the next release. No need to rush.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I need to do a version bump in the addon, otherwise home assistant won't show that an update exists, so I've been setting the addon versions independently of the releases

@DennisOSRM DennisOSRM merged commit bc713b8 into DennisOSRM:main Dec 10, 2023
1 of 2 checks passed
@DennisOSRM
Copy link
Owner

Thanks so much for the great work.

@DennisOSRM
Copy link
Owner

Just realized that the nightly build is also run on PRs. I am not sure we'd want that. What do you think?

@dominikandreas
Copy link
Collaborator Author

dominikandreas commented Dec 10, 2023

I think it's actually quite useful to see if a PR builds successfully and it also allows to quickly reference a build in docker hub for local testing in the home assistant addon.

Btw the branch name is added to the tag, so there's no conflict with the main builds

@DennisOSRM
Copy link
Owner

perfect. then that's more than fine. 😀

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.

2 participants