More configs for apprise and more #203
Conversation
Added configuration to set the logger level in config Also configs for toggling incomplete promotions and exceptions from apprise notifications Added chrome option --disable-http2 as suggested in (#185)
config.yaml.sample
Outdated
There was a problem hiding this comment.
IMO this .sample just complicates things needlessly. We should just have the file with defaults. Just isn't a common practice with config (with exception of private files usually).
config.yaml.sample
Outdated
| apprise: | ||
| summary: ON_ERROR | ||
| exceptions: True # True or False (Whether to send or not exceptions) | ||
| promotions: True # True or False (Whether to send or not incomplete promotions) |
There was a problem hiding this comment.
To be a bit clearer, it'd be nice to change this to apprise.notify.uncaught-exceptions and apprise.notify.incomplete-promotions. Reads a bit better/is clearer IMO.
.template-config-private.yaml
Outdated
There was a problem hiding this comment.
This should be restored, not having it increases the chances someone accidentally commits secret information and from personal experience makes it harder to develop as a result.
It's a bit jarring for people to move things back from config-private.yaml to config.yaml again as well.
main.py
Outdated
| traceback.format_exc(), | ||
| ) | ||
| if Utils.loadConfig().get("apprise", {}).get("exceptions", True): | ||
| Utils.sendNotification( |
There was a problem hiding this comment.
To simplify things, we could add an optional exception parameter (defaults to None). If an exception is provided in sendNotification, we can check the config to see if sending is enabled. If it is, we send. If it isn't, we just return.
|
I'm still not a fan of using two different config files based on whether they are private values or not, having a .sample file and having to rename its a part of setting up the script, which falls on the user, in the same way that an accounts.json.sample is provided and has to be renamed, why would config be different? the same way where if the accounts.json is not found it will be logged in the console, this could have been done for config.yaml too. If a default configuration must be provided then an hardcoded one could be used in case the file is missing, this is my opinion. Anyways i've reverted the changes related to the config and kept everything else in the lastest commit |
|
Another "logical" problem i've found when adding the config values, is when loadConfig is called, opening the file and reading it everytime a single value is needed is not optimal, now the script is small so this doesnt really matter performance wise, but still its not an optimal approach in my opinion, the configuration file should get loaded into memory when the script launches, then values are read when needed from a global variable containing all config values, i wanted to do it this way but the current way that the code is structured doesnt allow it without major refactoring my two cents |
The general idea is to put secrets/passwords/etc in a file that's gitignored. This prevents accidentally leaking confidential information. One problem that it creates, however, is that it's not exactly obvious what should be in the file that's gitignored (and you can't commit a file, and then gitignore it, once tracked all future changes are tracked). So, the general solution to that is to create a .template. file that can be copied into the new already .gitgnored location. I didn't give accounts.json + config-private.yaml much thought (honestly forgot until you mentioned), but I think a good practice eventually going forward would be to move accounts.json config to config-private.yaml in the future to make things a bit more straightforward. Having a generalized secrets/private file would be nice. And in regards to .sample.config.yaml, idk. We could provide defaults in-program and get rid of a version-controlled config.yaml completely, but IMO that's kinda problematic cause we'll have to document in the README or somewhere. Having the yaml itself is more self-documenting. |
Yeah, you can use MS-Rewards-Farmer/src/utils.py Line 38 in 0f0052c |
| apprise: | ||
| urls: | ||
| - "discord://{WebhookID}/{WebhookToken}" # Replace with your actual Apprise service URLs | ||
| default_geolocation: US # Replace with your country code https://en.wikipedia.org/wiki/ISO_3166-1_alpha-2 |
There was a problem hiding this comment.
Probably safe to keep this in config.yaml
|
Good stuff @Guido30, appreciate you fixing the timeout issue |
| @@ -205,8 +206,9 @@ def getCCodeLang(lang: str, geo: str) -> tuple: | |||
| try: | |||
| nfo = ipapi.location() | |||
| except RateLimited: | |||
| logging.warning("Returning default", exc_info=True) | |||
| return "en", "US" | |||
| geo = Utils.loadConfig("config-private.yaml").get("default_geolocation", "US").upper() | |||
There was a problem hiding this comment.
If this is explicitly set, I think we should prefer it to the location based on IP.
I've added #195 and #194, in the process i realized having both config-private.yaml and config.yaml was unnecessary, so now config.yaml is automatically ignored and only config.yaml.sample should be committed, the user should manually rename the file after configuring it as instructed in the updated README
I was getting the same problem as #185 so i tried using the --disable-http2 flag and it seems to have fixed the issue
i've also kinda fixed the ipapi ratelimited, it will still happen but now it instead of always using US as the default it will use the user configured country code, this does not block searches anymore for that day, the language is useless since its just the browser language, default will still be 'en'