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 support for multiple users #84

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

skatsavos
Copy link

Added support for multiple users

Added support for multiple users
@skatsavos skatsavos changed the title Add files via upload Add support for multiple users Oct 10, 2022
@skatsavos skatsavos mentioned this pull request Oct 10, 2022
@Schischu
Copy link

Hi @skatsavos , why do you want to remove the ENV variables?
That would break very likely a lot of Docker setups.

@skatsavos
Copy link
Author

It doesn't make sense to have env variable for Garmin credentials if you want to support multiple user/credentials. We may only leave it for backwards compatibility as single user, but it will need a lot of checks. I do know it will affect docker, as i only use the pip package variant.

Copy link
Collaborator

@longstone longstone left a comment

Choose a reason for hiding this comment

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

Thanks for the effort! I like the idea of working multiuser within the tool. I also see an issue where it comes to migration / docker exectuion.

To adress thoose issue, I think we could

  1. write a migration routine.
  2. better describe how the config file structure should look like (or even give a generate config file possibility
  3. still support single user (which will lead to the issue @skatsavos mentioned before.

@@ -50,34 +48,6 @@ optional arguments:
--verbose, -v Run verbosely
```

### Providing credentials via environment variables
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great if you could describe, that username and password are requested during first run.

username to log in to TrainerRoad.
--trainerroad-password TRAINERROAD_PASSWORD, --tp TRAINERROAD_PASSWORD
password to log in to TrainerRoad.
--withings-userid WITHINGS_USERID, --wuid WITHINGS_USERID
Copy link
Collaborator

Choose a reason for hiding this comment

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

For me, from the describtion its not quite clear:
Is this a withings specific user id or is it just a valu i want for this user? Should I geht this from the api or can I just use 42?

def get_garmin_username(self):
"""get Garmin username"""
if not self.withings.user_config[self.api_user_id].get("garmin_username"):
garmin_username = input("Garmin username : ")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question (for this and following input's): Can we use something like input with timemout?

@Olen
Copy link
Contributor

Olen commented Feb 8, 2023

Completely removing single user support and break Docker sounds like a bad idea to me. It is trivial to run multiple docker containers (one for each user), just make sure they don't share the .withings_user.json file (e.g. use different mountpoints for each user).

As far as I can see, this version still only sync one single account on each run, so you would still need to run one session for each user. So I don't really see the benefit of removing the environment and docker secrets handling.

For none-docker installs, you could just add multiple entries to withings_user.json and add a --withings-userid parameter to select which user you want to update, and still use --garmin-username and --garmin-password (or the env-vars/secrets) matching that withings-user.

This sounds like a much less intrusive and non breaking change.

@Olen
Copy link
Contributor

Olen commented May 11, 2023

This PR does so much more than just adding support for multiple users.
Why not simplify it and have the PR just provide the new option --withings-userid (and add a corresponding environment variable WITHINGS_USERID) with a default value "1" and handle that part.

I love the idea that you can make this work with multiple withings users mapped to one withings account, but breaking every other installation in the process is really not a good way forward.

The restructure to different files, addition of all the extra options that can be synced etc. are all fine, but should probably be separate PRs.

Removing Docker support completely does not make any sense at all to me.

Copy link
Contributor

@Olen Olen left a comment

Choose a reason for hiding this comment

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

You sould probably not post your client-id and secret to a public github repo...

NM. I see that this file is already in the repo. Still not a good idea though...

@shurane
Copy link

shurane commented Apr 5, 2024

I was thinking about this. I think it would be useful if the configuration info between Withings, Garmin, and even TrainerRoad were located in the same file. I.e. combining withings_user.json with the info for GARMIN_USERNAME and GARMIN_PASSWORD into a single file.

Then withings-sync could optionally take a file that would have all the info it needed to sync data between services. For a different user, you would just need a different configuration file. This could even be generated on the first run like how withings_user.json is being generated.

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.

5 participants