-
Notifications
You must be signed in to change notification settings - Fork 30
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
Configuration settable from outside #157
base: main
Are you sure you want to change the base?
Conversation
Added config argument to DbusService.__init__() to set the config file to use from outside This is necessary for SetupHelper installation method (config file is located in /data/conf/dbus-opendtu.ini to be preserverd during updates)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some verification if a config from outside is not reachable or similar. Otherwise it looks good to me. I assume you tested it beforehand, right?
Yes, I tested it. What du you mean with verification? Should I add a check for the instance like below? def _get_config(self):
'''return the cached configuration object, read it before if not already done'''
if self._config is None or not isinstance(self._config, configparser.ConfigParser):
self._config = configparser.ConfigParser()
self._config.read(f"{(os.path.dirname(os.path.realpath(__file__)))}/config.ini")
return self._config |
Some plausibility check will be helpful for some users E.g. file is readably and contains more than x bytes... Maybe something more... |
I didn't see any validation in the code before. So I just added the init() argument... Btw, the ConfigParser itself does not perform any checks; the documentation states: If none of the named files exist, the instance ConfigParser contains an empty data set. |
That's right. Since you started editing this part of the code, it might be a good idea to have checks in it. Don't you agree? I was only asking for it. Our software does not run without a config. Just check if the file is there and readable. No content checks. |
Just as a quick crosscheck? Wouldn't it be much easier to rename the example The implementation here breaks my installation with more than one DTU. In this MR; there is no option to query more than one DTU without changing the code after each update. |
When SetupHelper updates a package, a release archive is downdloaded and the old directory gets renamed (and removed when extraction was successful). So, during last update I lost my config inside the |
Did you read my comment above? SetupHelper removes the whole old version directory including your config.ini. Your PR is similar to one I did before I knew this behavior. I'll see if there is a solution with SetupHelper. |
I did, but I was not aware of the SetupHelper Package so I misunderstood it. So your approach is trying to make this addon SetupHelper compatible? Is this always approached by moving the config out of the Folder? PackageManager has a mechnism for backing up and restoring settings:
# SOME dbus Settings
# custom icons
# backing up gui, SetupHelper and PackageManager logs But I could not find any other reference of it |
Added config argument to DbusService.init() to set the config file to use from outside
This is necessary for SetupHelper installation method (config file is located in /data/conf/dbus-opendtu.ini to be preserverd during updates)