-
Notifications
You must be signed in to change notification settings - Fork 5
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
More options for App handling #33
More options for App handling #33
Conversation
Okay, I fixed the linting issues but I don't get what the problem with the test is. It's not entirely clear to me how these tests work. |
The test are run with molecule. It is really simple when you understand what it does. It creates a docker container, installs ansible there, downloads role dendencies there as well as the nextcloud role and then:
More or less, that's what happens. You can run the tests locally too if you install molecule ( |
Cool, thanks for the explanation! Currently locally it has some problems with docker but I'll figure it out. The CI passes now! |
Sorry for responding late. I think in the end it is better to simplify the logic and lose some functionality, like you propose here. Let's get this merged! |
Could you allow edits so that I can push some changes here too? |
Hm, that box is already checked...? |
I can't push some changes. Let's finish the discussion here and merge this and I will add them later to Let's go with the extra variable to also remove any "orphaned" apps. I think the functionality should be disabled by default to avoid surprises. It could be separate task to keep things simple. |
Hm, that's odd... Anyway, I'll implement the extra variable so we can merge. Thanks for the review! |
Done - not sure you have a more elegant solution than using |
cc478b9
to
e81923d
Compare
e81923d
to
0ab0d00
Compare
Is it too much to ask to use a separate task for the unknown apps instead of Do you want to try that? |
Haha, no it's of course not too much to ask. :) I'll just have to bend my brain a bit to get the conditional right... Let me give it a shot. |
Done - have a look and let me know if you like it. |
That took some work but it's now merged! Thanks again for your PR(s). |
This PR extends the functionality of the apps handling in this role. I have found that the manual installation procedure is too clumsy and I dropped it in favor of using
occ app:<...>
. This means (in contrast to what has been discussed in #8) it is not possible anymore to specify a version for an app to be installed. But we can now:nextcloud_apps
list.nextcloud_apps
list or setThe list of installed apps is refreshed after all install/removal happened and the remaining apps are updated to their latest available versions.