feat: add OctoApp support #454

2024-03-31T08:57:47+02:00

crysxd commented

2024-03-31 08:57:47 +02:00

(Migrated from github.com)

This PR adds support for OctoApp. I modified:

Main menu to reflect OctoApp
Update menu to reflect OctoApp
Remove menu to reflect OctoApp
Redme

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

This PR adds support for OctoApp. I modified: - Main menu to reflect OctoApp - Update menu to reflect OctoApp - Remove menu to reflect OctoApp - Redme My scripts are copied and modified from the OctoEverywhere ones, as my plugin is a fork of OctoEverywhere

dw-0 commented

2024-03-31 10:17:44 +02:00

(Migrated from github.com)

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.

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

2024-03-31 10:55:49 +02:00

(Migrated from github.com)

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.

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

2024-03-31 11:52:01 +02:00

(Migrated from github.com)

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.

> 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.

dw-0 (Migrated from github.com) requested changes 2024-03-31 13:27:30 +02:00

README.md

dw-0 (Migrated from github.com) commented

2024-03-31 13:16:26 +02:00

You might want to point the URL in the a-tag to the correct ressource.

scripts/octoapp.sh

dw-0 (Migrated from github.com) commented

2024-03-31 13:24:58 +02:00

      blank_line
      echo -e "| The setup will apply the same names to OctoApp        |"
      blank_line
      echo -e "| Please select the number of OctoApp instances to      |"
      echo -e "| install. Usually one OctoApp instance per Moonraker   |"
      echo -e "| instance is required, but you may not install more    |"
      echo -e "| OctoApp instances than available Moonraker instances. |"
      bottom_border

dw-0 (Migrated from github.com) commented

2024-03-31 13:27:16 +02:00

This function is declared twice in this file. This is coming form the OctoEverywhere script, where this function is also declared twice. We can simply remove that one here.

crysxd (Migrated from github.com) reviewed 2024-03-31 17:07:37 +02:00

README.md

crysxd (Migrated from github.com) commented

2024-03-31 17:07:37 +02:00

Thank you, too much copy and paste :D

crysxd (Migrated from github.com) reviewed 2024-03-31 17:08:08 +02:00

scripts/octoapp.sh

crysxd (Migrated from github.com) commented

2024-03-31 17:08:08 +02:00

Removed. I ping Quinn about the duplicate function in OE, but as both are identical there is not much harm done

crysxd commented

2024-03-31 17:10:40 +02:00

(Migrated from github.com)

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

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

dw-0 (Migrated from github.com) approved these changes 2024-03-31 17:13:54 +02:00

crysxd commented

2024-03-31 17:15:14 +02:00

(Migrated from github.com)

Thank you! :)

dw-0 commented

2024-03-31 17:15:35 +02:00

(Migrated from github.com)

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 :)

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 :)

🚀 1

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: h3n3/kiauh#454