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

Multiple update clients #63

Closed

Conversation

jordanruthe
Copy link
Contributor

First attempt at allowing moonraker to have multiple client statements. Also enabling updates for git repositories (use case being KlipperScreen). Let me know any questions you may have.

@Arksine
Copy link
Owner

Arksine commented Jan 9, 2021

Its pretty good. I think that it should be possible to add a "git" based client without adding the "repo_info" and "dist_info" sections, putting, putting everything necessary in the [update_manager_client] section. I just need to add some flexibility to how the configuration is parsed. Let me give it some thought on the best approach then we can move from there.

@jordanruthe
Copy link
Contributor Author

What if we just got rid of the repo_info and dist_info sections all together? The supplemental configuration could be converted to update_manager_client and loaded as is. While it doesn't have a path or env path like I have documented, we can an if statement to fill in this information for moonraker/klipper. So it'd look something like...

[update_manager_client moonraker]
type: git_repo
origin: https://github.com/arksine/moonraker.git
requirements: scripts/moonraker-requirements.txt
venv_args: -p python3
install_script: scripts/install-moonraker.sh
python_dist_path: /usr/lib/python3/dist-packages
env_package_path: lib/python3.7/site-packages
python_dist_packages:
 gpiod

Otherwise I can modify the update manager to have flexibility and pull from the update_manager_client section while leaving the other two sections

… in one section

Signed-off-by: Jordan Ruthe <jordan.ruthe@gmail.com>
@cadriel
Copy link

cadriel commented Jan 11, 2021

Is there an option to perhaps leave existing configs in a working state? I know I have a lot of users that now rely on this update feature - and having it break for all of'em might be painful.

What would happen in that scenario, would it update fine - then on restart, throw an incorrect configuration error? Or would it fail silently until the next update when things would not work?

Maybe another option would be to have the update itself change the config - so it'd be transparent to users.

@Arksine
Copy link
Owner

Arksine commented Jan 11, 2021

I think that there are ways of doing this without breaking config and without changing the API too much. For example, we can keep moonraker.update.client and simply add an argument to identify additional clients.

What I'm thinking is that we can support something similar to Jordan's latest proposal, however we will keep the "first" client configured in the main [update_manager] section. Then we can transition away from it at a later date. I would also like to abstract a bunch of the options in the "git_repo" type away from the user, as they wouldn't be relevant for client repos.

Signed-off-by: Jordan Ruthe <jordan.ruthe@gmail.com>
@jordanruthe
Copy link
Contributor Author

I put the client section back in. For the update_manager_client block, I'm not sure that I could do more abstraction. Here is a sample block I've tested, and I feel like everything is needed except possibly the venv_args.

[update_manager_client KlipperScreen]
type: git_repo
path: /home/pi/KlipperScreen
env: /home/pi/.KlipperScreen-env/bin/python
origin: https://github.com/jordanruthe/KlipperScreen.git
requirements: scripts/KlipperScreen-requirements.txt
venv_args: -p python3
install_script: scripts/KlipperScreen-install.sh

@Arksine
Copy link
Owner

Arksine commented Jan 20, 2021

Ok, I had some time to work on this. I took this pull request and made some changes to it:

  1. The client section is named [update_manager client client_name]. I added the space so Moonraker would treat it as a "mux" section, which prevents it from spamming errors my attempting to load each section as a plugin.
  2. There are two "types", git_repo and web
  3. I made the Updater objects responsible for config parsing and validation. I removed some of the checks on the origin. It is possible that they don't have a .git extension, and it may be possible that repo is hosted on Gitlab or some other service.
  4. I don't create a new endpoint for each client. Doing so makes the API unpredictable, so instead one may pass a name argument to the client endpoint, ie /machine/update/client?name=klipperscreen.
  5. I removed documentation on the python dist package option. This functionality is probably specific to Moonraker and confusing for users, so I don't think its necessary to put it there. However if you have the need to symlink dist packages in your python env I can add it back.

As before, this will change the response to /machine/update/status. Instead of reporting a 'client' field it will report the name of each Client.

The branch is dev-update-manager-multiclient. @meteyou @cadriel When this is merged it will break update functionality in your clients, so I will hold off until you are ready. @jordanruthe I haven't tested this for a "git repo" client, but it should generally work the same as before. I'll try to install KlipperScreen on a test machine and make sure it behaves.

@cadriel
Copy link

cadriel commented Jan 20, 2021

Sounds good - I'll take a look some time this week time permitting.

Cheers!

@cadriel
Copy link

cadriel commented Jan 21, 2021

Ok, I've taken a quick look. I have another branch ready to push as a patch for when this gets merged.

Otherwise all good this side!

@jordanruthe
Copy link
Contributor Author

Looks like it works properly for updating a git repo. However, the web api documentation needs to be updated in your branch, specifically: https://github.com/Arksine/moonraker/blob/dev-update-manager-multiclient/docs/web_api.md#update-client

It's missing the reference to name when you are trying to update a [update_manager client *] with /machine/update/client

meteyou added a commit to mainsail-crew/mainsail that referenced this pull request Jan 23, 2021
Signed-off-by: Stefan Dej <meteyou@gmail.com>
@Arksine
Copy link
Owner

Arksine commented Jan 24, 2021

Multi-client functionality has now been added

@jordanruthe
Copy link
Contributor Author

Thanks!

@jordanruthe jordanruthe deleted the multiple_update_clients branch May 17, 2021 02:11
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.

None yet

3 participants