search

dw-0 / kiauh

Klipper Installation And Update Helper
GNU General Public License v3.0
3.09k stars 452 forks source link

feat: add OctoApp support #454

Closed crysxd closed 3 months ago

crysxd commented 3 months ago

This PR adds support for OctoApp. I modified:

My scripts are copied and modified from the OctoEverywhere ones, as my plugin is a fork of OctoEverywhere

dw-0 commented 3 months ago

Hi! Thank you for your contribution. As you might have seen (or maybe not), im currently in the process of a full KIAUH rewrite. That means migrating the whole script to Python 3. At the same time there will be some fundamental structural changes. This includes how i consider "non core components" to be implemented in KIAUH. There was a feature request for adding your plugin to KIAUH which you can see here: https://github.com/dw-0/kiauh/issues/408

I mentioned that i rather see this integrated as a, what i will probably call "Community Extension", for KIAUH. An idea and poc of how this would look like can be seen here: https://github.com/dw-0/kiauh/issues/330 In the v6 dev branch there are already 2 extensions implemented, so if you're interested about the "look and feel" feel free to check it out there as well. As i notices, some extensions might require not only install and remove, but also update. So i guess i will add an optional update entry point to the module.

Long story short: Im hesitant in merging this PR, as it will also require a rewrite into becoming a KIAUH community extension soon. And it might be too early to also start migrating this PR to a community extension for v6 already. Although the extension feature is already implemented in v6 and looks fine to me, i would still consider it WIP. So just to let you know, this PR has to stay on hold for a while.

crysxd commented 3 months ago

I understand that the timing of this request is not ideal and I also took my sweet time to work on this.

I'm also not in an immense rush, so if I get an idea on what "soon" means for the community extensions I'm happy to wait and rework my integration. I just don't want to get in a situation where this now doesn't get merged and the community extensions take another 6 months to make their way into the production version of KIAUH as I get a fair amount of requests on adding OctoApp to KIAUH.

As my modifications here are a 1:1 copy of the OctoEverywhere integration, there is also not a lot of overhead on migrating my changes to any new system as the OE changes would need to get the same treatment already.

My proposal would be: If the community extensions are right around the corner I'm happy to wait and then integrate via this path. But if the community extensions are still more than 1-2 months out, I'd prefer to get this merged now and I'm then happy to do the extra work required to migrate them over. If you consider the v6 somewhat stable I can do so already now.

dw-0 commented 3 months ago

My proposal would be: If the community extensions are right around the corner I'm happy to wait and then integrate via this path. But if the community extensions are still more than 1-2 months out, I'd prefer to get this merged now and I'm then happy to do the extra work required to migrate them over. If you consider the v6 somewhat stable I can do so already now.

I think that would be fine with me. I know you put in the work and holding this PR until i am finished with v6 (earlier those extensions won't make it to production) wouldn't be ideal. I will have a look at this PR later today and review it. And as soon as v6 comes live (it is not stable right now), that feature can get migrated to a community extension.

crysxd commented 3 months ago

I completed all requested changes, thank you for checking this so quickly! There is also one small change, I now call the install entry "OctoApp for Klipper" to make it more clear that this is indeed a Klipper addon and not for OctoPrint. I did not change the other menus as the text was too long. If you want to I can revert the change or copy it to the other menus, making them a tad wider

crysxd commented 3 months ago

Thank you! :)

dw-0 commented 3 months ago

Thank you!

I did not change the other menus as the text was too long. If you want to I can revert the change or copy it to the other menus, making them a tad wider

I think it is fine for now. We can leave it as is and will tackle that when v6 is around the corner :)