Config: Read GitHub and GitLab tokens from Config File #306

Closed sonic2kk closed 7 months ago

This PR implements the backend for #305, it does not contain any GUI changes.

Overview

Right now we pass GitHub (and GitLab) access tokens via an environment variable. This works, but it has to be appended every time. On one hand, this is not so bad because tokens expire, but on the other hand until they expire (which could be months) if you want to keep using them to avoid a rate limit, you have to append them as an environment variable. For Flatpak, this can be configured from a tool to set Flatpak environment variables, such as Flatseal.

However it was requested to read these values from the config file instead. This PR implements the ability to read these variables from the config file instead of having to pass them as environment variables. Right now the main benefit to this is that it's more "coupled" to ProtonUp-Qt and means a user doesn't have to fiddle with Flatpak permissions or edit Desktop entry environment variables. But in future this will have the larger benefit of being configurable from a GUI, we can expose it on the GUI pretty easily I think.

Implementation

Replacing Environment Variables

With this PR, a user can set PUPGUI_GHA_TOKEN or PUPGUI_GLA_TOKEN in the config.ini, under the pupgui2 section (this matches the environment variable names), and ProtonUp-Qt will read this when setting the values for the MainWindow web_access_tokens dictionary. This replaces the environment variables.

The reason I went with replacing environment variables is that in future, we will expose this on the GUI, and so would negate the need for the environment variable. Having an option to force a config option via an environment variable is something I did consider. For example, a user might want to pass PUPGUI_GHA_TOKEN as an environment variable.

I don't see an intuitive way to know which should take priority -- Would a user be more likely or less likely to expect the environment variable to take priority? For example if no value is in the config they might expect the environment variable to take priority, but if they set it they might think that the environment variable should be ignored. Another user might have an entirely different idea. Therefore, I thought it best to simply rip the band-aid off and go all-in on using the config file to define these variables only. Complicating the logic to check for both a config file or environment variable option doesn't seem worth it to me.

Also, afaik we don't allow other config options to be overridden with environment variables, and I don't think the breaking compatibility is super important here, especially if we document this on release and have the option to set these values on the GUI after release.

For this reason, I went with replacing the environment variables with the config file values.

Generic Config Read/Write Function

No prizes for guessing this one, I went with my favourite approach for this: Generic functions!

When I was looking at how to implement this feature, I noticed the pattern of parsing the config file using util functions like config_blah_blah. But the code was very repetitive between functions, basically boiling down to "if option in config section, read and return value, otherwise write option to config section if not falsey". So I made this a generic function for this called read_update_config_value. It does exactly what the other config parsing functions do for theme and advanced mode, but generically.

This function takes an option (i.e. PUPGUI_GHA_TOKEN), a value for this option (i.e. gha-123foo456bar), a optional section (defaults to pupgui2 since it's the most common), and an optional config file (defaults to CONFIG_FILE, but I allowed this to be overridden in case we ever need to modify another config file, it was a low-cost change that helps extensibility).

The idea behind this functions is that other util functions for specific config files can call it with the option, value, and section information populated. In this PR, we have two new util functions that call it are config_github_access_token and config_gitlab_access_token. These function names follow the convention mentioned earlier. These are basically "wrapper" functions, with the idea being, we call these functions so that we don't have to know the key/section name when calling these functions. The functions themselves are aware of that, and all we have to do is call them and say "hey, write this value out for the config file value you control".

This means config_gitlab_access_token for example is always aware of the config file option it has to update and the section it has to update it in, but whatever is calling config_gitlab_access_token doesn't have to know that at all. I thought this was a cleaner way to architect it. Hope that explanation makes sense :-)

This should be generic enough that we can re-use it for config_advanced_mode and config_theme, but I didn't test it because I wasn't sure if it should go in this PR. If acceptable though I am happy to do that in this PR :slightly_smiling_face:

Section and Option Naming

These config file values go into the same config.ini that all other options to into, as defined in constants.py. I chose to use the pupgui2 section as it seemed like other options went there. If we want a separate section though, that's fine by me :smile:

The INI currently seems to have a "naming scheme" of all lowercase, singleword options (i.e. advancedmode). The names I chose for the access tokens goes against this, and it was simply in the interest of matching the environment variable names. But it can be easily changed if preferred, maybe something simple like githubtoken and gitlabtoken. Once these options are on the GUI though this will really not matter, and because of the way the functions are written, only the function controlling a given option needs to have the option name change.

Some of the functionality of the web tokens may be pending changes following #302, but for now this PR does populate web_access_tokens with the values from the INI file. If desired we can use the new function I created to read/update config file values for theme and advanced mode, or it can go in a separate PR following this one.

Thanks!

Had to rework read_update_config_value because it was prioritising reading the config file instead of writing out to it if we passed a value.

It looks a bit less clean with the indent but I discovered this issue when testing refactoring config_theme with this function. It worked, and I accidentally pushed a commit that contained it :sweat_smile: The part has been reverted now, and initial functionality from this PR Is still in-tact.

The function also works for toggling advanced mode, but toggling install_directory is a bit less straightforward I think. It seems like it has a lot more tightly-coupled logic. config_custom_install_location is also pretty beefy and cannot be as easily refactored it seems.

Actually, it was possible to modify util.py#install_directory to use the new function.

Implementation

```python3 # modified install_directory function from protonup 0.1.4 def install_directory(target=None) -> str: """ Read/update config for the selected install directory Write target to config or read from config if target=None Return Type: str """ # Write installdir option to config file if target and target.lower() != 'get': if target.lower() == 'default': target = POSSIBLE_INSTALL_LOCATIONS[0]['install_dir'] if not target.endswith('/'): target += '/' read_update_config_value('installdir', os.path.expanduser(target), section='pupgui') # Read installdir option from config file else: target = os.path.expanduser(read_update_config_value('installdir', None, section='pupgui')) print(f'target is {target}') if target in available_install_directories(): return target elif len(available_install_directories()) > 0: install_directory(available_install_directories()[0]) return available_install_directories()[0] return '' ```

I did notice that it seems to get called several times (from the print I can see it gets called 4-5 times on startup, and when changing launchers), probably not really a big deal though.

For config_custom_install_location, it might be possible to do this. We could have to make multiple calls to read_update_config_value unless we refactor the writeout to take maybe a dictionary and update multiple values (where the dictionary is option/value pairs that we iterate over or something) but I think that's excessive. We could make a helper function to remove an option from a config file, and after that it shouldn't actually be that difficult to refactor as well.

Potential, minimally-tested refactor:

```python3 # This function can go anywhere, it's just up here for clarity :-) def remove_config_value(option: str, section: str = 'pupgui2', config_file: str = CONFIG_FILE): config = ConfigParser() if not option or not section: return config.read(config_file) if config.has_option(section, option): config.remove_option(section, option) with open(CONFIG_FILE, 'w') as cfg: config.write(cfg) # Will make two I/O calls to config, whereas before only one I/O call was made # Two to either update, remove, or read the values # Maybe this is fine? def config_custom_install_location(install_dir=None, launcher='', remove=False) -> Dict[str, str]: """ Read/update config for the custom install location Write install_dir, launcher to config or read if install_dir=None or launcher=None Return Type: dict Contents: 'install_dir', 'display_name' (always ''), 'launcher' """ if install_dir and launcher and not remove: read_update_config_value('custom_install_dir', install_dir, section='pupgui2') read_update_config_value('custom_install_launcher', install_dir, section='pupgui2') elif remove: remove_config_value('custom_install_dir', section='pupgui2') remove_config_value('custom_install_launcher', section='pupgui2') elif os.path.exists(CONFIG_FILE): install_dir = read_update_config_value('custom_install_dir', None, section='pupgui2') launcher = read_update_config_value('custom_install_launcher', None, section='pupgui2') if install_dir and not install_dir.endswith('/'): install_dir += '/' return {'install_dir': install_dir, 'display_name': '', 'launcher': launcher} ````

Thanks! :tada:

With this PR, a user can set PUPGUI_GHA_TOKEN or PUPGUI_GLA_TOKEN in the config.ini, under the pupgui2 section [...] This replaces the environment variables. Would a user be more likely or less likely to expect the environment variable to take priority?

I'd probably implement it such that it prefers the environment variables over the config. This way, people who have already set up environment variables don't need to change anything. It also allows us to already merge this PR before the UI is available without breaking anything.

Generic functions!

Now that we have even more config option, this makes sense. Great!

This should be generic enough that we can re-use it for config_advanced_mode and config_theme, but I didn't test it because I wasn't sure if it should go in this PR. If acceptable though I am happy to do that in this PR 🙂

It's probably more clean to do it in another PR.

The INI currently seems to have a "naming scheme" of all lowercase, singleword options (i.e. advancedmode). The names I chose for the access tokens goes against this, and it was simply in the interest of matching the environment variable names

I think we can change the names to match the lowercase naming scheme as we don't need the PUPGUI prefix anymore. Something like github_api_token should do.

Some of the functionality of the web tokens may be pending changes following #302, but for now this PR does populate web_access_tokens with the values from the INI file

That shouldn't break anything, as self.web_access_tokens isn't touched there. PS: It's available now in https://github.com/DavidoTek/ProtonUp-Qt/pull/307

Actually, it was possible to modify util.py#install_directory to use the new function. I did notice that it seems to get called several times (from the print I can see it gets called 4-5 times on startup, and when changing launchers), probably not really a big deal though.

install_directory() is used a bit like a global variable. I'm also not sure if this is the best way. The only advantage I see there is that if we have many more config options in the future (and we implement install_directory with read_update_config_value), it would allow us to lock the config file with a Mutex to prevent a corrupted file. Just a thought though.

We could have to make multiple calls to read_update_config_value unless we refactor the writeout to take maybe a dictionary and update multiple values

I think multiple calls will be fine :)

I'd probably implement it such that it prefers the environment variables over the config. This way, people who have already set up environment variables don't need to change anything. It also allows us to already merge this PR before the UI is available without breaking anything.

Last point especially is very good, not sure how many people run ProtonUp-Qt from git (I do it out of habit at this point :sweat_smile:) but for those that do, and in case this goes in a release without the UI portion being completed, like you said it'll preserve compatibility.

I'll push the changes suggested in the comment after some light testing but I don't see any problems with that approach.

It's probably more clean to do it in another PR.

Once this is merged I'll take a look at doing a refactor, theme and advanced mode should be very low-risk changes.

install_directory() is used a bit like a global variable. I'm also not sure if this is the best way. The only advantage I see there is that if we have many more config options in the future (and we implement install_directory with read_update_config_value), it would allow us to lock the config file with a Mutex to prevent a corrupted file. Just a thought though.

This is a very good thought. If/when we have the "Settings" dialog there is a potential that we may get requests (or ideas) for more settings to add, so I could somewhat foresee a scenario where we could have more config options. But yeah, maybe best to leave these for now :-)

At least, I'll prioritise refactoring the advanced mode and theme options. The code snippets in this PR could also serve as a bit of a starting point, if we ever decide to refactor the other functions in future.

I think we can change the names to match the lowercase naming scheme as we don't need the PUPGUI prefix anymore. Something like github_api_token should do.

Sounds good, I'll change that as well :-)

Updated to use environment variables, tested and it seems to work as expected.

Also updated the config option names, since they were only used in this PR, there is no impact to changing thee name - unless someone else tested this PR outside of us and added the old values themselves, then they'll have to rename them, but that's an edge case :-)

Also tested it, works as expected.

I made a small adjustment to the messages in the message box. I removed the with PUPGUI_GHA_TOKEN in \'config.ini\' part as the config key is now called different. This also won't require the message to be re-translated again. Instead, I added the necessary details to the linked comment: https://github.com/DavidoTek/ProtonUp-Qt/issues/161#issuecomment-1358200080

Thanks very much, this will be merged.

DavidoTek / ProtonUp-Qt