mailinabox
mailinabox copied to clipboard
mail-in-a-box
Make Nextcloud optional
This pull request will add the ability to skip installing Nextcloud if the DISABLE_NEXTCLOUD environment variable is set to "0".
Even though Nextcloud's contacts and calendar features can be useful to some, Nextcloud adds an additional attack surface to a MiaB server, when you combine that with the fact that not everyone needs these features in the first place, making Nextcloud an optional feature starts becoming a logical decision to make.
Thanks for taking the time to review this PR, feel free to suggest improvements/fixes.
Good work! I, for one, would find this useful especially as Nextcloud seems to be so fragile for many users. Personally I never use Munin either!
Thank you :)
My suggestion would be to alter the fail2ban jails so that the Nextcloud configuration is added in to a base config so that code is not duplicated.
Honestly, I don't know how fail2ban would handle Nextcloud if it isn't installed, so I decided to play it safe by adding two configs. Now that I think about it, I will make a single base fail2ban config without Nextcloud, and the additional Nextcloud configs will be added by the setup/system.sh script if it detects that DISABLE_NEXTCLOUD isn't set to "1".
One addition I would find helpful would be a script (or code that detected a current Nextcloud configuration) that removed any Nextcloud configuration from a current MIAB that had previously installed Nextcloud.
That's a great suggestion! I will work on this soon.
Also how does a user choose not to install Nextcloud? Should there be a question in the setup?
Right now, it's by setting an environment variable before running the install script. The current input_box function (from setup/functions.sh) doesn't have a yes/no mode, so I would have to implement a yes/no function, and I'm not sure if @JoshData is ok with that.
Now that I think about it, I will make a single base fail2ban config without Nextcloud, and the additional Nextcloud configs will be added by the setup/system.sh script if it detects that DISABLE_NEXTCLOUD isn't set to "1".
Remember that in bash/shell "1" is usually false and "0" is true. This might be confusing with the variable name DISABLE_NEXTCLOUD.
The current input_box function (from setup/functions.sh) doesn't have a yes/no mode, so I would have to implement a yes/no function, and I'm not sure if @JoshData is ok with that.
I would be surprised if @JoshData accepted this PR anytime soon as it removes something a lot of people use and has lots of changes but I think I am right he has said in the past he kind of regrets adding Nextcloud in the first place (but don't quote me on that, lets see what he says!). Adding a yes/no question function should be simple enough to implement and I can help you with that if needed once I have time.
Alright, I've just committed a fix to the two fail2ban config files issue. Now, the relevant Nextcloud config is stored in a separate file, and will be appended to the main config if the user wants it.
Remember that in bash/shell "1" is usually false and "0" is true. This might be confusing with the variable name DISABLE_NEXTCLOUD.
I will take a look at this, thanks.
I would be surprised if @JoshData accepted this PR anytime soon as it removes something a lot of people use and has lots of changes but I think I am right he has said in the past he kind of regrets adding Nextcloud in the first place (but don't quote me on that, lets see what he says!).
Well it isn't removing anything, it's just making Nextcloud optional.
Adding a yes/no question function should be simple enough to implement and I can help you with that if needed once I have time.
Thanks, I'll let you know if I need any help.
One addition I would find helpful would be a script (or code that detected a current Nextcloud configuration) that removed any Nextcloud configuration from a current MIAB that had previously installed Nextcloud.
I would be surprised if @JoshData accepted this PR anytime soon as it removes something a lot of people use and has lots of changes but I think I am right he has said in the past he kind of regrets adding Nextcloud in the first place
How would this flow work out? and would it be easy enough to and/or make sense to implement?
Install script detects that this is a completely new installation, then prompts user to either install or skip installation of NC (maybe even Munin could be added to this?) or Install script detects that there is an existing install of MiaB, it would then prompt user to leave NC or uninstall it. or Install script detects that there is an existing install of MiaB without NC, it would then prompt user to leave NC uninstalled or install it.
That way all of the possible scenarios are taken into account. This also gives the user complete flexibility to install or remove NC either initially, or when there is a new version of MiaB released. I also think that this way would lead to a easier decision to implement by @JoshData as it eliminates the issue in the comment by not forcing removal.
I just implemented a Yes/No function and a Yes/No question to see if the user wants to disable Nextcloud or not.
The user no longer needs to set an environment variable manually.
As for the removal of Nextcloud, that will probably be in a separate script.
Should the install script ask the user if he wants to enable Nextcloud, instead of if he wants to disable it?
Should the install script ask the user if he wants to enable Nextcloud, instead of if he wants to disable it?
My preference would be to ask to enable but perhaps as it is currently installed by default it should be kept as disable. @JoshData will hopefully make a decision when he has time to review this PR.
Alright, I've just added the option to remove Nextcloud. It basically asks the user if he wants to remove Nextcloud if it detects its presence. The script takes a backup of Nextcloud's files, then proceeds to delete the original files, after that, it removes Nextcloud's dependencies as well. I haven't tested the script myself, but it should work. The configuration files (nginx, fail2ban, etc...) will be generated accordingly because when the user chooses to remove Nextcloud, DISABLE_NEXTCLOUD will be set to 0 as well. What do you think, @ctrl-i?
I won't have a chance to test this for a while as I don't currently have a MIAB instance I can test this on but I hope to add this to my MIAB fork when I can. I have been wanting to remove Nextcloud for a while and replace it with Radicale and this PR will allow this to progress. I'll also be following your example and removing Munin.
I haven't checked yet but does your script allow everything @alento-group asked for? @alento-group is a frequent and helpful contributor to the MIAB community.
Thank-you for your good work on this PR. I hope @JoshData will take some time to review this and accept it especially as a lot of the problems MIAB often faces are caused by Nextcloud. Thank-you also for your quick responses and edits to the comments I raised, it has been a pleasure working with you on this.
If Nextcloud is disabled or removed do we still need the Contacts/Calendar page in the HTML admin pages?
I won't have a chance to test this for a while as I don't currently have a MIAB instance I can test this on but I hope to add this to my MIAB fork when I can. I have been wanting to remove Nextcloud for a while and replace it with Radicale and this PR will allow this to progress. I'll also be following your example and removing Munin.
Good luck with your fork :)
I haven't checked yet but does your script allow everything @alento-group asked for? @alento-group is a frequent and helpful contributor to the MIAB community.
Yes, here are his suggestions and how I implemented them:
Install script detects that this is a completely new installation, then prompts user to either install or skip installation of NC (maybe even Munin could be added to this?)
It doesn't detect if this is a completely new installation, but it should ask the user if he wants to not install Nextcloud if it isn't present.
Install script detects that there is an existing install of MiaB, it would then prompt user to leave NC or uninstall it.
It doesn't detect an existing installation of MiaB, but it detects an existing installation of Nextcloud, so it should ask if Nextcloud should be removed or not.
Install script detects that there is an existing install of MiaB without NC, it would then prompt user to leave NC uninstalled or install it.
If Nexcloud isn't installed, the user would be asked whether he wants to leave it disabled or not.
Thank-you for your good work on this PR. I hope @JoshData will take some time to review this and accept it especially as a lot of the problems MIAB often faces are caused by Nextcloud. Thank-you also for your quick responses and edits to the comments I raised, it has been a pleasure working with you on this.
It has been a pleasure working with you too! Your reviews helped me a lot. Thanks :)
If Nextcloud is disabled or removed do we still need the Contacts/Calendar page in the HTML admin pages?
Probably not, I'll take a look at this and see if it can be easily removed.
Added a commit to make sure that the Contacts/Calendar page won't show up if the user didn't enable Nextcloud.
Thanks for taking the time to create the PR.
I think this introduces a complexity to the code base that would be better served by creating a clean fork.
I think this introduces a complexity to the code base that would be better served by creating a clean fork.
Isn't Nextcloud the complexity though? By removing Nextcloud we would be making the code base simpler. This PR gives users an option to keep Nextcloud as some users would not like to see it go. As you know, Nextcloud has been the source of many recent problems and this potentially fixes these issues and would make it easier to drop Nextcloud in the future.
Isn't Nextcloud the complexity though? By removing Nextcloud we would be making the code base simpler. This PR gives users an option to keep Nextcloud as some users would not like to see it go. As you know, Nextcloud has been the source of many recent problems and this potentially fixes these issues and would make it easier to drop Nextcloud in the future.
@yodax I have to agree with @ctrl-i ... rather than introducing complexity it introduces the ability to make a clean break from NC. Yes, maybe there is some 'complexity' in the code to implement this, but looking at the bigger picture, it eliminates something that many do not want nor use, and even @JoshData has indicated that he somewhat regrets adding to this project, yet leaving it there for those who have use for it and do want it. I would actually like to see this go one step further and remove munin ... yes, I am sure that for a few (1-2% maybe?) there is a use for munin ... but I doubt it is important to 98% of the project's users. However, adding munin to the same procedures would absolutely cause confusion as many of those who are interested in this project would have zero clue what it even is... so, let's not talk about munin any further, but let's continue with what this pr accomplishes. Thanks!
I think OwnCloud/NextCloud needs a little bit of grace against our usual reluctance to scope creep the project.
Normally less is more but since we cant rip it out completely (some people use it) and we don't have a time machine, I go against my normal stance and strongly agree this PR in some form should be accepted.
I admit I am biased because I faced many hours battling NextCloud breaking my upgrade last month but that is essentially the entire point.
NextCloud is excellent for this that want it an a whole extra unnecessary headache and attack surface to cater for those who dont.
I appreciate the replies and see that people are passionate about this. And I really do understand why you would want this feature in. That I use Nextcloud personally doesn't matter in this case, I'm happy enough maintain my own fork (with some extra's that didn't get merged and some personal extension points).
Isn't Nextcloud the complexity though? By removing Nextcloud we would be making the code base simpler.
The setup now contains many more if statements scattered throughout. This doesn't make the product easier to maintain. When adding a new feature or supporting an old one, there are now two flows to keep into account for as long as this is in. It would also still contain Nextcloud and all the complexity that goes with it.
As you know, Nextcloud has been the source of many recent problems and this potentially fixes these issues and would make it easier to drop Nextcloud in the future.
For the project this PR will not makes these issues go away. Nextcloud will still be installed on a large set of installs. It will only add to them since the complexity of the installer has gone up.
I would be fine with dropping Nextcloud from MIAB, I would simply fork and keep it in my install. I don't think this is the approach. If there is a desire for a "just a mailserver/dns" that desire has merit. The decision should be taken, do we do this here or somewhere else.
@alento-group rather than introducing complexity it introduces the ability to make a clean break from NC.
A PR that makes a clean break would be much simpler than this current PR. It would involve a migration script that would remove the current install and drops all the components. This however would break MIAB for many people. So the argument that this is a good place to start when removing nextcloud isn't true imho. Removing nextcloud from miab is (from a coding perspective) much simpler and doesn't need all these if statements.
So to recap, I'm fine with either keeping it, or removing it. My point is this hybrid PR causes a lot of extra effort to review and test every future PR. From a support perspective there's now also more that can go wrong when helping someone.
In my humble opinion, I would like to keep Nextcloud. I really like that Nextcloud is a part of MiaB. It really gives the freedom to operate without any services of the Big5.
I'm grateful to MIAB for the opportunity to learn more things technical but I'm no way near the level of people building the future of MIAB here on Github. Most people like me would not usually follow discussions on GitHub.
So if you make NextCloud optional or if you do away with it in MIAB, please explain very clearly and with enough advance notice how we - the silent crowd with moderate technical skills - can manage the transition. I've come to use the NextCloud associated with MIAB a lot. Thanks in advance.
So to recap, I'm fine with either keeping it, or removing it. My point is this hybrid PR causes a lot of extra effort to review and test every future PR. From a support perspective there's now also more that can go wrong when helping someone.
I can't say I agree with that, nothing is being added with this PR if I remember correctly other than a few if statements. Maintaining Nextcloud on the other hand seems to cause more issues and problems for users. I do not see where the "lot of extra effort" is and every new addition involves some extra effort.
If you want an easier to maintain project then Nextcloud should be removed just look at the problems many users had at the last major upgrade for that.
(Like you I maintain my own fork with extras I want included.)
Maintaining Nextcloud on the other hand seems to cause more issues and problems for users.
If you want an easier to maintain project then Nextcloud should be removed just look at the problems many users had at the last major upgrade for that.
That is exactly my point, this PR doesn't remove nextcloud therefore doesn't solve any problems for the project. It will help some users that choose to disable it. The pressure of maintaining nextcloud doesn't go away for the project as a whole.
I can't say I agree with that, nothing is being added with this PR if I remember correctly other than a few if statements.
There are a 120 added lines for this alone, so it's more than a few if statements. That in itself doesn't worry me. It's that with every new PR you need to ask yourself, what if nextcloud is disabled? What if nextcloud is uninstalled in the future?
Then come testing time when pushing out a release, you need to test: Clean install, Clean install without Nextcloud. Upgrade, Upgrade without Nextcloud.
I would be much more hesitant to make changes to the project with this PR in. Removing Nextcloud completely would make maintenance and support much easier. (I'm not in favor of removing at all, just making the point that it would be the technical superior option)
With regards to the PR itself, I think z-push still has the backends for card/caldav configured in the zpush backend configuration. I wonder too, if z-push will function properly without card/caldav. This would require rigorous testing.
I would be much more hesitant to make changes to the project with this PR in.
That would be unfortunate but it would not stop me or lots of others I don't think.
Removing Nextcloud completely would make maintenance and support much easier. (I'm not in favor of removing at all, just making the point that it would be the technical superior option)
This I agree with.
The users I maintain a MIAB instance for use Nextcloud but my own personal MIAB fork has it removed already as I do not use it.
With regards to the PR itself, I think z-push still has the backends for card/caldav configured in the zpush backend configuration. I wonder too, if z-push will function properly without card/caldav. This would require rigorous testing.
What is the difference between testing and rigourous testing? :-)
Ultimately we need @JoshData to decide on whether to keep Nextcloud once and for all. That way forks can happen and the most popular one will win out.
What is the difference between testing and rigourous testing? :-)
z-push has many different behaviors depending on which client you connect with. Simply pairing it with an android phone will not tell you if it works with an iphone, windows mail or outlook. That's what I mean with rigorous testing, not just a simple connection test with one device.
IMHO whatever happens we should not be actively encouraging a fork as any kind of solution until all other options are exhausted. As other have already said I concur that @JoshData should decide and we live with that. I hope this PR is accepted in some form but I will happily pay the price of not getting the option if it means no forks happen.
I hope me mentioning a fork is not seen as a threat to leave, that's not what I meant. If this is merged, I'm quite content to keep helping and no hard feelings too. 😄
I think my views on it are clear, I'm happy with whatever @JoshData decides.
This is pretty good. There are a few issues, but it'll have to come back to giving feedback when I have more time.
I very much agree with this. I've never touched NextCloud on my MIAB instance and never plan on it. It's just chewing up resources for no additional benefit. Same thing with Munin monitoring which I could take up adding it as "optional" as well.
