More options for App handling

nkakouros-original / ansible-role-nextcloud

An Ansible role to install Nextcloud

GNU General Public License v3.0

11 stars 5 forks source link

More options for App handling #33

Closed simonspa closed 5 years ago

simonspa commented 5 years ago

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:

install apps: just add them to the nextcloud_apps list.
remove apps: either remove them from the nextcloud_apps list or set
```
nextcloud_apps:
  - name: bookmarks
    state: absent
```

Disable or enable apps (including shipped system apps):

nextcloud_apps:
  - name: survey_client
    state: disabled
  - name: admin_audit
    state: enabled

The list of installed apps is refreshed after all install/removal happened and the remaining apps are updated to their latest available versions.

simonspa commented 5 years ago

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.

nkakouros commented 5 years ago

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:

runs yamllint
runs ansible-lint
runs ansible-playbook usiing the molecule/default/prepare.yml and molecule/default/playbook.yml playbbooks.
re-runs molecule/default/playbook.yml to check for idempotency.

More or less, that's what happens. You can run the tests locally too if you install molecule (sudo pip install molecule) and then do molecule test in the role's root directory.

simonspa commented 5 years ago

Cool, thanks for the explanation! Currently locally it has some problems with docker but I'll figure it out.

The CI passes now!

nkakouros commented 5 years ago

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!

nkakouros commented 5 years ago

Could you allow edits so that I can push some changes here too?

simonspa commented 5 years ago

Hm, that box is already checked...?

nkakouros commented 5 years ago

I can't push some changes. Let's finish the discussion here and merge this and I will add them later to master.

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.

simonspa commented 5 years ago

Hm, that's odd...

Anyway, I'll implement the extra variable so we can merge. Thanks for the review!

simonspa commented 5 years ago

Done - not sure you have a more elegant solution than using set_facts, but it's working for me.

nkakouros commented 5 years ago

Is it too much to ask to use a separate task for the unknown apps instead of set_fact? One task would loop nextcloud_apps and do whatever is there and another would loop the unknown apps and remove them given that the nextcloud_remove_unknown_apps variable is true.

Do you want to try that?

simonspa commented 5 years ago

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.

simonspa commented 5 years ago

Done - have a look and let me know if you like it.

nkakouros commented 5 years ago

That took some work but it's now merged! Thanks again for your PR(s).