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

Add config option for update information #357

Closed

Conversation

uneart
Copy link

@uneart uneart commented Nov 26, 2024

Fixes #334

Proposal for an additional option to disable the update information coordinator. This would fix repair warnings for users not willing to give Sys.Modify permissions to the Proxmox user/group.

@dougiteixeira please review if this goes into the right direction from your point of view.

I tested this on my staging Home Assistant instance and it seems to work as expected.

Open tasks

  • Translation EN
  • Testing
  • Docs / changelog

@uneart uneart mentioned this pull request Nov 26, 2024
1 task
@uneart uneart marked this pull request as ready for review November 27, 2024 21:26
@Sab44
Copy link

Sab44 commented Nov 29, 2024

Thanks a lot for taking care of this issue!
I tested your changes, the repair issue is no longer showing up 👍
However there seems to be a small issue with the strings in the config flow, where "update_enable" is using the key rather than the value to display text, see attached screenshot:
Screenshot 2024-11-29 at 10 38 56
Again thanks, and let's hope we can get it merged into the integration!

@uneart
Copy link
Author

uneart commented Nov 29, 2024

@Sab44 Hm I had the issue first as well but had fixed it by adding the proper string translation. What language are you using?

Can you please check if there are any errors showing up in your logs that might help debugging this?

@Sab44
Copy link

Sab44 commented Nov 29, 2024

HA is in english. All the other strings on this form appear fine.
I noticed that in the en.json you don't have the update_enable string after line 23 and 26. But tbh even after adding them there it still didn't show up for me...

@uneart
Copy link
Author

uneart commented Nov 29, 2024

@Sab44 you're right, I missed that one. Added it now. I think, after updating, you would need to restart HA in order to see any effects. If you have a testing environment, maybe you can try :-)

@Sab44
Copy link

Sab44 commented Dec 1, 2024

Hey @uneart it's showing up properly now 👍
Good job. @dougiteixeira if you could take a look at this it'd be appreciated. Thanks!

@dougiteixeira
Copy link
Owner

This implementation is not complete, let me think and see if another approach works. In the next few weeks I will come back with a definitive solution.

Thanks!

@dougiteixeira dougiteixeira marked this pull request as draft December 2, 2024 02:03
@dougiteixeira
Copy link
Owner

dougiteixeira commented Dec 8, 2024

The code to ignore the missing permission repair has been fixed (#371). I think it is simpler and more practical for the user than adding another configuration option.

Previously, even ignoring the repair would reappear after restarting or reloading the integration. This has now been fixed.

Thanks for the contribution anyway.

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.

Sys.modify authorization
3 participants