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

Nextcloud addition #145

Merged
merged 3 commits into from
Oct 14, 2019
Merged

Nextcloud addition #145

merged 3 commits into from
Oct 14, 2019

Conversation

bennysp
Copy link
Contributor

@bennysp bennysp commented Feb 17, 2018

I have taken the owncloud-official.json and created a new nextcloud-official.json that pulls the official latest image. Can you please add this to the registry?

@bennysp
Copy link
Contributor Author

bennysp commented Feb 18, 2018

I should also mention that I have installed this in my local Rockstor with no issues found.

@phillxnet
Copy link
Member

It would be great to have this, long awaited, Nextcloud Rockon merged. Anyone up for reviewing it?

Linking back to the relevant forum thread for context:
https://forum.rockstor.com/t/linuxserver-ios-nextcloud/3101/20

@magicalyak
Copy link
Contributor

I'll try to review tonight, sorry about the delay!

@magicalyak
Copy link
Contributor

@bennysp Fantastic job, I really like the container and can see it's use immediately!
One issue I had though: the database name is confusing for new users, can we expand the information there? I assume it must be named nextcloud but what is the purpose for alternative names? Does the docker container use whatever name you throw at it and if so, why wouldn't we just hardcode nextcloud as the DB name? I'm fine leaving it to prompt but users will likely pause and wonder what to name it. Adding the default of nextcloud would likely solve this or just removing the prompt and using nextcloud as the name. Can you let me know about this and I can then approve and ping for merge?
Can we also link the URL to the docker page on github, it will help anyone customizing (this request is minor though)

@magicalyak
Copy link
Contributor

URL for docker container, this can be in the description or url link ideally - https://github.com/nextcloud/docker

@bennysp
Copy link
Contributor Author

bennysp commented Apr 6, 2018

Sounds good @magicalyak , will update and agree on just defaulting on the DB name. I will update!

@daniel-illi
Copy link
Contributor

@bennysp the nextcloud-official.json rockon file looks good to me.

But I'm wondering if it is a good idea to force sqlite to be used as database engine.
According the nextcloud documentation it is only recommended to use sqlite for testing or for single user setups as long as no sync client is used.

IMO it would be preferable to include MySQL/MariaDB/Postgresql in the rockon file, or at least not to set the SQLITE_DATABASE env variable to let the user decide himself.

@bennysp
Copy link
Contributor Author

bennysp commented Apr 6, 2018

okay, thanks for the feedback @daniel-illi and @magicalyak . I will be looking into these and get back to you. Really appreciate you looking at this!

@doenietzomoeilijk
Copy link
Contributor

If we're throwing prefs around - yes, I definitely would like the choice of not using sqlite, since in my experience NC doesn't scale beyond one user with it. On the other hand, I already have a MySQL container running, so shoehorning in another one would be a waste of resources on my server.

Personally, I'd rather see a separate rockon for a database and a separate ones for whatever apps that uses that database.

Of course that's a bit less inexperienced-user-friendly, so either the "power users" need a separate rockon, which means more work and maintenance for the maintainers, or there should be some way to switch behavior in one rockon. That's possible but a lot more work.

So yeah, +1 for not forcing sqlite. ;-)

@doenietzomoeilijk
Copy link
Contributor

One more thought - Rockstor already has a perfectly serviceable pgsql running. Maybe it's worth looking at that?

@magicalyak
Copy link
Contributor

I’m fine approving with just the default dB but if you can make it use a better dB that’s great. Let me know if you run into trouble. Generally most rockons use their own database even if it’s a linked container. End users can always create a custom dockerfile to do this but it’s a bit advanced for regular users to run one rockon first and link to another. They’d need to create the database and then name it. Or you need to fork the repo and make it create the database. If you need help let me know. There are some examples already in the repo you can view to see how linked containers work.

@doenietzomoeilijk
Copy link
Contributor

No, I didn't mean I wanted to fork or anything, just pushing some thoughts out there. And yes, several Rock-ons seem to supply their own DB, which is new-user-friendly, but not very resource-friendly. Personally, I'm considering not using Rock-ons at all, and just going with a bunch of custom docker-compose files - I'm certainly comfy enough with Docker to make that work.

As I said, another option is to add some flexibility to the Rock-on system wrt database usage etc. However, that's a fair amount of work, for which I don't have the time, unfortunately.

@daniel-illi
Copy link
Contributor

IMO rock-ons should be end user friendly above all and provide sane defaults.
Power users will know how to tweak things anyway or roll their own solution, i.e. with docker-compose as mentioned by @doenietzomoeilijk (as I am doing as well).

It is not that much a resource overhead to bundle the database engine with the rockon, but it simplifies the setup for end users. I'd keep the required database adminitration tasks for end users to a minimum, bundle a database that is suitable for the task, and preconfigure as much as possible.

Regarding a sane default for nextcloud, sqlite does not qualify. Once the user realizes that he has outgrown the db engine capabilities, he will have to migrate to a suitable db engine (which he might not know how to do) or start all over again. That is if he realizes why his nextcloud instance is not performing as it should.

@bennysp
Copy link
Contributor Author

bennysp commented Apr 11, 2018

So, if I interpret the feedback correctly... Default it to PGSQL, but let the Rock-on do all the assumptions. If there is a power user involved, they can tweak as necessary, but this will cover the 80/20 rule.

Agreed?

@magicalyak
Copy link
Contributor

I leave the decision to you honestly. I think that’s a fair compromise. However if SQLite is all that works than go for it and note that it’s really for small setups or testing and not heavy use. If you get pgsql working awesome and even better.

I didn’t mean to suggest anyone was wrong here I’m just looking at functionality over perfection if that makes sense.

@doenietzomoeilijk
Copy link
Contributor

I agree that sqlite is the easiest get-it-working backend, but even for one user it starts falling apart rather quick once you start synching a device or two and using the various apps, at least it did for me. As @daniel-illi pointed out, that's a tough problem to solve for a less knowledgable user once they run into it, even though NC actually has a procedure for it. Maybe run this by @phillxnet and/or @schakrava to see how they think about using the "main" pgsql db for this kind of thing - in the end it might be a safer bet to just toss a mysql or mariadb container in there and suck up the memory hit that might bring.

@magicalyak
Copy link
Contributor

It’s also a pain to update a container and not break existing installs. One thought here is renaming the rockon to -SQLite or -test so its out there and usable and then taking time on the pgsql. I’ll actually help on that if you like maybe even try it tonight. I know I’ve gotten some complex setups working before.

@bennysp
Copy link
Contributor Author

bennysp commented Apr 11, 2018

Funny @magicalyak , I was thinking the same thing I think. A default "out of box" to match the official docker image and one with mysql/pgsql and "-pgsql" at the end of it.

@phillxnet
Copy link
Member

@bennysp I now have pr merge capability and am in the process of extending my Rockstor 'duties' to managing this repo. As such I would really like to get this one sorted.

@magicalyak @daniel-illi @doenietzomoeilijk Thanks for bringing your expertise to this pr. A much appreciated effort.

quoting @bennysp
"So, if I interpret the feedback correctly... Default it to PGSQL, but let the Rock-on do all the assumptions. If there is a power user involved, they can tweak as necessary, but this will cover the 80/20 rule.

Agreed?"

I'm thinking this is the consensus, but maybe for another pull request as there is still the argument that adhering to the official docker image is valuable.

If we have something that from multiple accounts is likely to fall apart soon after less than trivial use we are asking for trouble and then bestowing that on our less technical users.

However I like the idea of the test / light variant to stick with upstream 'official' variant and we could express in the description that it is for single user applications or the like. And as @magicalyak stated we can offer a full blown 'multi user' variant (with a more capable db embedded). This is of course more work but I think it's important that we get these in if at all possible.

So does that mean we can merge this one as the 'light variant' with a description to indicate it's limitations. And then add a more robust variant via another pull request.

Further input to finalise our consensus on this one would be great.

And again much apologies for the unreasonable delay on this repo. Hopefully I can make a difference going forward by taking on some of the pull requests maintenance.

So in short it looks like I can merge this if it get a final review and has some description / name change to indicate it's light / limited capacities.

Thanks everyone for moving us closer towards getting this one (and it's potential more capable sibling) in finally.

@phillxnet phillxnet added help wanted change requested A change has been requested labels Feb 2, 2019
@FroggyFlox
Copy link
Member

I'll chip in on this one as having an all inclusive Nextcloud Rock-on is one of the main reasons I got interested in the Rock-on system in general.

I agree pretty much with everything else said above so I won't repeat that, but I did want to add that I like the idea of having an offer for as many levels of complexity as we can as long as we keep it as simple as possible for non-power users. In this context, I like having a Rock-on as the "light" variant, one with an included MariaDB database, and one without database set up to allow a power user to use a separate Rock-on for database such as the MariaDB Rock-on we have.
On the latter, I would like to add that my current work on improving Rock-on / Docker networking is intended to ease just that: allowing one to connect one Rock-on to another, which supports the idea of having an official Nextcloud Rock-on without an included database.

@HBDK
Copy link
Contributor

HBDK commented Jul 21, 2019

So i played around with this rockon this week and it seemed to work alright, but i ultimately ended up making my own using the Linuxserver.io image, it seemed to play better with external storage.

if you are concerned about keeping it easy for newcomers i think you should consider making sure the container uses https.

@dmcharpentier
Copy link

To weigh in on this as well would it be possible to have rockstor create a general MySQL container and then create users and databases automatically for each container that needs it? I know that ups the complexity... But it would remove the bottleneck on resources and at the same time solve many problems. I also think that sqlite is not a good path moving forward with this. In my testing it can get really slow very fast if you upload a bunch of files on there even with just one person and no sync.

@magicalyak
Copy link
Contributor

@dmcharpentier it is possible but we'd be at a point where you'd almost want to brand that as a subset of rockons that depend on that mysql and then the app rockons would work with it. I would start doing this as a personal project with the jsons in a local rockstor install and then ask about a branded option. For now, I think having an MySQL/MariaDB container rockon and the app rockon saying you need a standalone and could use the former DB container would be fine, it would just be two rockons with descriptions indicating this.

@phillxnet
Copy link
Member

@bennysp @magicalyak @daniel-illi @doenietzomoeilijk @FroggyFlox @HBDK @dmcharpentier
I'm wanting to wake this pr up again as we have continued interest in getting at least 'a' NextCloud Rock-on in. And unless anything has changed, I haven't checked, this is an official one. But we have numerous reports here that's it's 'single user light duty' orientated.

Given we have reached almost an impasse I'd really like to get the ball rolling again on this one re:

So does that mean we can merge this one as the 'light variant' with a description to indicate it's limitations. And then add a more robust variant via another pull request.

Shall we pop this one in, after adding "appropriate for single user light duty installs"?

Or have things moved on in the mean time? All input welcome as I'm just not up on OwnCloud / NextCloud development but from what I've heard it would be a real win for Rockstor to offer a NextCloud and so it worth making the final push on this pull request if it get us started again. I'd also like to see a 'proper' multi-use Postgres backed Rock-on variant but one step at a time may get us closer to that. As long as we name / describe our Rock-ons appropriately.

Linking to an recent forum post expressing NextCloud interest:
https://forum.rockstor.com/t/upgrade-from-owncloud-official/6409

@phillxnet phillxnet closed this Oct 4, 2019
@phillxnet phillxnet reopened this Oct 4, 2019
@doenietzomoeilijk
Copy link
Contributor

Shall we pop this one in, after adding "appropriate for single user light duty installs"?

I have not run this rockon myself, so I can't vouch for the actual state of things working as they should etc, but the idea of having a few versions of the software, one with all the bits attached and one that's more of a byob-affair, sounds like a decent plan.

By the look of things, this should grab the latest version, which right now is 17. Not sure how things are handled wrt updates, but that's a different kettle of fish. A quick skim over the image used and the volume mounted, I'd say it should work.

@FroggyFlox
Copy link
Member

I have not run this rockon myself, so I can't vouch for the actual state of things working as they should etc, but the idea of having a few versions of the software, one with all the bits attached and one that's more of a byob-affair, sounds like a decent plan.

I agree with you.

By the look of things, this should grab the latest version, which right now is 17.

I just tested and it indeed pulls down Nextcloud 17 (from the logs):

 Initializing nextcloud 17.0.0.9 ...

I'll try to get a proper review in as soon as possible.

Copy link
Member

@FroggyFlox FroggyFlox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bennysp thanks again for this rock-on!
I could verify it works as intended and provided a few suggestions based on the conversation above.

Note that you would need to also add this json to root.json so that it can be parsed with the other rock-ons.

    "Nextcloud-Official": "nextcloud-official.json",

Thanks again!

Comment on lines +22 to +28
"environment": {
"SQLITE_DATABASE": {
"description": "Enter the name of the database you would like to create.",
"label": "SQLite DB Name",
"index": 1
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following @magicalyak's comment (#145 (comment)), I agree that we may prefer to set the db name to make it even easier to the user.
The suggestion below would thus need to replace the entire "environment" object:

Suggested change
"environment": {
"SQLITE_DATABASE": {
"description": "Enter the name of the database you would like to create.",
"label": "SQLite DB Name",
"index": 1
}
}
"opts": [
[
"-e",
"SQLITE_DATABASE=nextcloud-sqlite"
]
],

"volumes": {
"/var/www/html": {
"description": "Choose a Share for Nextcloud. Eg: create a Share called nextcloud-all for this purpose alone.",
"label": "App Storage",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment:
To avoid any confusion with the Nextcloud APPS (which could be mounted on a separated volume), we can edit this label:

Suggested change
"label": "App Storage",
"label": "Storage",

}
},
"version": "latest",
"description": "Next generation open source Enterprise File Sync and Share",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly state:

  • use of SQLite database --> lightweight use only
  • does NOT use HTTPS.
  • docker image used, with link
Suggested change
"description": "Next generation open source Enterprise File Sync and Share",
"description": "Next generation open source Enterprise File Sync and Share. <p>This rock-on uses a simple SQLite database and may thus be limited to <em>single-user or lightweight use</em>. Similarly, it does <strong>not</strong> use encryption.</p><p>Based on the official Nextcloud docker image: <a href='https://hub.docker.com/_/nextcloud' target='_blank'>https://hub.docker.com/_/nextcloud</a>.",

"more_info": "<p>No smbclient installed by default. If you would like to enable this you can connect to your container and use <code>apt-get install smbclient -y</code> to install.</p>",
"ui": {
"slug": ""
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add support for "Add Storage" post-install:

Suggested change
},
},
"volume_add_support": true,

@phillxnet
Copy link
Member

@FroggyFlox If we don't hear back from @bennysp soon I'm tending towards committing your changes and publishing. We can always amend later now it's mostly in shape and up front about it's capabilities.

@phillxnet phillxnet merged commit b714fda into rockstor:master Oct 14, 2019
@phillxnet
Copy link
Member

@bennysp @magicalyak @daniel-illi @doenietzomoeilijk @HBDK @dmcharpentier
@FroggyFlox Merging this as is prior to @FroggyFlox suggestions as I'm unable to commit the suggestions as I suggested previously and due to delays / indecision primarily on my part we have lost engagement from @bennysp.
So give this Rock-on is as yet not included in root.json and we don't publish from GitHub directly I'm inviting @FroggyFlox to open a pr with their suggestions. That way we can preserve what attribution and info we have in this pr and get this one out finally.
Apologies to all for this important Rock-on not receiving the attention it deserved.

FroggyFlox added a commit to FroggyFlox/rockon-registry that referenced this pull request Oct 14, 2019
phillxnet added a commit that referenced this pull request Oct 14, 2019
Update Nextcloud-official according to #145 comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change requested A change has been requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants