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

More options for App handling #33

Merged
merged 12 commits into from
Nov 12, 2019

Conversation

simonspa
Copy link
Contributor

@simonspa simonspa commented Nov 6, 2019

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
Copy link
Contributor Author

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.

tasks/core/apps.yml Show resolved Hide resolved
@nkakouros
Copy link
Collaborator

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 (sudo pip install molecule) and then do molecule test in the role's root directory.

@simonspa
Copy link
Contributor Author

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

The CI passes now!

defaults/main.yml Outdated Show resolved Hide resolved
tasks/core/apps.yml Outdated Show resolved Hide resolved
@nkakouros
Copy link
Collaborator

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
Copy link
Collaborator

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

@simonspa
Copy link
Contributor Author

Hm, that box is already checked...?

@nkakouros
Copy link
Collaborator

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
Copy link
Contributor Author

Hm, that's odd...

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

@simonspa
Copy link
Contributor Author

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

@nkakouros
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@nkakouros nkakouros merged commit dff5cdc into nkakouros-original:master Nov 12, 2019
@nkakouros
Copy link
Collaborator

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

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

2 participants