nkakouros-original / ansible-role-nextcloud

An Ansible role to install Nextcloud
GNU General Public License v3.0
11 stars 5 forks source link

Change how preference update is handled #32

Closed simonspa closed 4 years ago

simonspa commented 4 years ago

This is an attempt to fix #31.

The new approach is to take the full current preference set, apply the nextcloud_config on top and then check if it differs from the current configuration. If yes, we update, if no, we skip.

I now put the merging of the two configs into the condition - if you find it more clear to add a separate

- name: Merge local configuration and instance configuration
  set_fact:
    _nextcloud_updated_preferences: "{{ _nextcloud_old_preferences | combine(nextcloud_config, recursive=True) }}"

let me know, for me either is fine.

The tests still need to be adapted to check if this works properly, by manual checks worked fine (probably need to catch a case where nextcloud_config is empty/ not defined).

nkakouros commented 4 years ago

I added some comments, could you take a look?

simonspa commented 4 years ago

The approach has a small caveat: all "global" settings need to be placed under system:

nextcloud_config:
  system:
    log_rotate_size: '104857600'

to be picked up correctly. I have added a check for the result of occ config:import which should fail the task if invalid keys were found.

simonspa commented 4 years ago

I see that you had this already before. We could thing of splitting it in the two main categories:

to make it a bit more legible - but I don't insist, I think it's also fine the way we have it now.

simonspa commented 4 years ago

Hm, the tests somehow don't seem to know the search filter.

nkakouros commented 4 years ago

I see that you had this already before. We could thing of splitting it in the two main categories:

* `nextcloud_config_system`

* `nextcloud_config_apps`

to make it a bit more legible - but I don't insist, I think it's also fine the way we have it now.

The role actually was like that in the beginning. And I would treat nextcloud_config_system differently for some reason. Then I realized that for Nextcloud system and app settings are of the same type, so I merged the two into nextcloud_config. The system key is what Nextcloud uses to hold the system settings, so I just follow the Nextcloud organization.

For someone not so familiar with nextcloud I can see this becoming a point of confusion. I think explaining the system key in the comments as well as providing a link to the official documentation where these config options are explained/listed would be a nice addition.

simonspa commented 4 years ago

For someone not so familiar with nextcloud I can see this becoming a point of confusion. I think explaining the system key in the comments as well as providing a link to the official documentation where these config options are explained/listed would be a nice addition.

Alright, I'll leave that for another PR - there are some more items in your default/main.yml which need an update.

simonspa commented 4 years ago

Is there anything I should fix here before we can merge?

nkakouros commented 4 years ago

Sorry, I will have to take a look a bit later on this.