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

WIP: Rework Readme file #53

Merged
merged 22 commits into from
May 24, 2017
Merged

WIP: Rework Readme file #53

merged 22 commits into from
May 24, 2017

Conversation

SnowMB
Copy link
Contributor

@SnowMB SnowMB commented Mar 15, 2017

Complete rework of the readme file.

I tried getting clearer on many topics that raised questions in the past. Hope it's understandable.

Before it get's merge we should do changes to the update/unpack script and solve #23

Please review @pierreozoux

Copy link
Member

@pierreozoux pierreozoux left a comment

Choose a reason for hiding this comment

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

I think that then, we can get rid of the .example folder, right?

README.md Outdated
Now you can access Nextcloud at http://localhost/ from your host system. To make your nextcloud installation available from the internet you must map the port of the container to your host:

```console
$ docker run -p 80:80 -d nextcloud
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to avoid giving insecure instructions, and link to SSL instructions here. What is your opinion on that?

@pierreozoux
Copy link
Member

@tilosp how did you add your commit on top?
I did checkout locally the PR, and pushed it, and now I have this: https://github.com/nextcloud/docker/commits/pr/53... Sorry for the mess :)

@tilosp
Copy link
Member

tilosp commented Mar 21, 2017

@pierreozoux You need to push to the source branch of the pull request, in this case SnowMB:master.
Your mess is fixed 😃

@tilosp tilosp mentioned this pull request Mar 26, 2017
@tilosp tilosp closed this Mar 26, 2017
@tilosp tilosp reopened this Mar 26, 2017
@tilosp
Copy link
Member

tilosp commented Mar 26, 2017

Closed and reopened to manually trigger a Travis build

@SnowMB
Copy link
Contributor Author

SnowMB commented Apr 5, 2017

@pierreozoux

We should keep the examples folder (I reference it quite a few times in the Readme). I would even like to extend in the future and maybe split it into subfolders.

On the SSL part: I wanted to show the important things step by step (Exposing port, adding database, adding volumes, adding encription). For tinkering in the local network, exposing port 80 is fine. But we can add some more warning text or so.

@SnowMB
Copy link
Contributor Author

SnowMB commented Apr 25, 2017

Rebased the whole thing and adapted to #65.

Added a part on migrating. Content should be ready for merge, just have to rework same typos.

Please review @tilosp, @pierreozoux.

README.md Outdated
When using docker-compose:

```console
$ docker-compose up -d --pull
Copy link
Member

@tilosp tilosp Apr 26, 2017

Choose a reason for hiding this comment

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

I can't find the --pull option in the docs https://docs.docker.com/compose/reference/up/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, that slipped. Fixed

@tilosp
Copy link
Member

tilosp commented Apr 26, 2017

We should wait for #65 to make it into the docker library before merging this PR.
And we need to update https://github.com/docker-library/docs/tree/master/nextcloud.

README.md Outdated
```console
$ docker pull nextcloud
$ docker stop <your_nextcloud_container>
$ docker rm <your_nextcloud_container>
$ docker run -d nextcloud
Copy link
Member

Choose a reason for hiding this comment

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

we need to add -v webroot:/var/www/html because we need to persist the version.php in it for the new upgrade system

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I'll rework the non-compose commands to reflect actual use with volumes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tilosp I'm a bit confused right now. Do you mean the upgrade script needs the old installation to perform a propper upgrade?

What would happen if I just mount the folders listed below and then pull a newer image?

$ docker run -d nextcloud \
-v apps:/var/www/html/custom_apps \
-v config:/var/www/html/config \
-v data:/var/www/html/data \
-v theme:/var/www/html/themes/<YOUR_CUSTOM_THEME>

I think this:
It would not find any version.php and perform a "new installation" wich copies all the installation files but does not overwrite the existing data files.

The only problem might be, that occ:upgrade is not triggered.

We could fix this in the upgrade script or by simply remove suggesting fine grained volume control from the readme.

Have I missed something?

Copy link
Member

Choose a reason for hiding this comment

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

With the new upgrade system we need to always have the /var/www/html volume.
And if the user wants to have the data, config, apps or themes in a different volume he can additionally add the corresponding volume.

README.md Outdated
- `/var/www/html/custom_apps` installed / modified apps
- `/var/www/html/config` local configuration
- `/var/www/html/data` the actual data of your Nextcloud
- `/var/www/html/theming` theming/branding
Copy link
Member

Choose a reason for hiding this comment

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

I think the folder is called themes

Copy link
Contributor Author

@SnowMB SnowMB May 10, 2017

Choose a reason for hiding this comment

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

Indeed, where is my mind? ;)
But I need to rework that section, too. Mounting the whole folder is not necessary because of the examples folder. Also #72

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@topas-rec
Copy link

topas-rec commented May 15, 2017

Hello everyone,

I just found nextcloud beside owncloud and was really impressed - until I saw what was published regarding docker.
This readme update hooked me up again and I'll try to get it running. (The Apache image ran within a few minutes, but owncloud also gave me ssl and no security warnings in the admin menu - in a few minutes ;-))

Nice work - many thanks. I hope it gets released for everyone on the main page of docker soon.

It might help to change the words "your-name" in line 232 and 233 to "name of your container" but this is just cosmetics. I'll try to reply when I got it working or gave up. (I'm a perfect test object since my docker experience is 4 days joung)

@tilosp
Copy link
Member

tilosp commented May 16, 2017

@pierreozoux please review

Copy link
Member

@pierreozoux pierreozoux left a comment

Choose a reason for hiding this comment

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

Really nice work!
Beside the SSL part, I'm really happy, this looks promising! (And feel free to ignore my SSL comment if you feel like)

This work can inspire docker-library/docs#238

@tianon if you could also give a quick review, it would be nice!

Thanks @ALL

PS: it has to be squashed before merge

README.md Outdated
volumes:
- db:/var/lib/mysql
environment:
- MYSQL_ROOT_PW=...
Copy link
Member

@pierreozoux pierreozoux May 16, 2017

Choose a reason for hiding this comment

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

I'd remove the ..., then if you don't put anything, docker-compose throw an error (this is the desired behavior, people putting their own password, and not using any default.

README.md Outdated

```console
$ docker run -p 80:80 -d nextcloud
Copy link
Member

Choose a reason for hiding this comment

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

As already said, I'm not a big fan to ease non secure deploy. If you want to keep this, I'd add a section "development/testing" where it explains how to map such insecure way. And this section should be after the SSL section.

Copy link
Contributor Author

@SnowMB SnowMB May 17, 2017

Choose a reason for hiding this comment

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

Each part contains a line about how that setup does not contain ssl encryption, I can make it bold if you like 😁. Since nextcloud itself complains about unsecure connection when you connect via http, I think it should be fine.

But I'll have a look at removing port mappings from the example and move it to a new part as you proposed.

README.md Outdated
app:
image: nextcloud
ports:
- "80:80"
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above about the insecure deploy

README.md Outdated
web:
image: nginx
ports:
- "80:80"
Copy link
Member

Choose a reason for hiding this comment

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

same comment as above about the insecure deploy

README.md Outdated


# Adding Features
A lot of people want use additional functionality inside their nextcloud installation. If the image does not include the packages you need, you can easily build your own image on top of it.
Copy link
Member

Choose a reason for hiding this comment

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

want to use

@SnowMB
Copy link
Contributor Author

SnowMB commented May 17, 2017

Yeah definetly squash merge. 😀

@SnowMB
Copy link
Contributor Author

SnowMB commented May 17, 2017

@pierreozoux Adressed your review. I removed all port mappings from the examples and created a new section called Connect your Nextcloud to the internet. This should remove your concerns 😄.

Copy link
Member

@pierreozoux pierreozoux left a comment

Choose a reason for hiding this comment

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

I think we are good to go :) Thanks a lot for the efforts!

@topas-rec
Copy link

topas-rec commented May 18, 2017

Hello everyone,

I sent some time testing the example (docker compose). I would like to give feedback and talk about an error of the setup.
Feedback first:

  • I didn't know that I have (or am allowed) to change the docker compose file.
  • I didn't know that I have to set passwords of the db container.
  • I didn't knew what to download from the .example directory. I took all files. I started sudo docker-compose up (sudo because I didn't setup groups for docker - I'm personally fine with using sudo)
  • I wasn't able to set the environmental variable DOMAIN from the docker compose file in the docker command so I changed it in the docker compose file. (I didn''t knew this)
  • I had to figure out myself that I need to set a valid domain name - localhost didn't worked for me.
  • I didn't knew that a valid email is necessary for lets encrypt

After fixing these things I got mostly all services up and running.

Now to one issue that I have after I was able to reach the proxy via my ddns (no-ip) domain:
THe nginx web server - not the proxy - shows the following errors when a client connects:

PHP message: {"reqId":"J8dbbzhk41yQ6bR4\/K\/+","remoteAddr":"172.18.0.6","app":"PHP","message":"fopen(\/var\/www\/html\/config\/config.php): failed to open stream: No such file or directory at \/var\/www\/html\/lib\/private\/Config.php#230","level":3,"time":"2017-05-18T19:09:06+00:00","method":"GET","url":"\/","user":"--","version":""} PHP message: {"reqId":"J8dbbzhk41yQ6bR4\/K\/+","remoteAddr":"172.18.0.6","app":"PHP","message":"chmod(): No such file or directory at \/var\/www\/html\/lib\/private\/Config.php#233","level":3,"time":"2017-05-18T19:09:06+00:00","method":"GET","url":"\/","user":"--","version":""}" while reading response header from upstream, client: 172.18.0.6, server: , request: "GET / HTTP/1.1", upstream: "fastcgi://172.18.0.3:9000", host: "[...].ddns.net" 2017/05/18 19:09:06 [error] 5#5: *1 FastCGI sent in stderr: "PHP message: {"reqId":"J8dbbzhk41yQ6bR4\/K\/+","remoteAddr":"172.18.0.6","app":"PHP","message":"chmod(): No such file or directory at \/var\/www\/html\/lib\/private\/Log\/File.php#119","level":3,"time":"2017-05-18T19:09:06+00:00","method":"GET","url":"\/","user":"--","version":""}" while reading upstream, client: 172.18.0.6, server: , request: "GET / HTTP/1.1", upstream: "fastcgi://172.18.0.3:9000", host: "[...].ddns.net" 2017/05/18 19:11:08 [error] 5#5: *56 FastCGI sent in stderr: "PHP message: {"reqId":"tJuu66dEOjjKrdvZuVC3","remoteAddr":"172.18.0.1","app":"PHP","message":"touch(): Unable to create file \/var\/www\/html\/config\/config.php because Permission denied at \/var\/www\/html\/lib\/private\/Config.php#229","level":3,"time":"2017-05-18T19:11:08+00:00","method":"GET","url":"\/","user":"--","version":""} PHP message: {"reqId":"tJuu66dEOjjKrdvZuVC3","remoteAddr":"172.18.0.1","app":"PHP","message":"fopen(\/var\/www\/html\/config\/config.php): failed to open stream: No such file or directory at \/var\/www\/html\/lib\/private\/Config.php#230","level":3,"time":"2017-05-18T19:11:08+00:00","method":"GET","url":"\/","user":"--","version":""} PHP message: {"reqId":"tJuu66dEOjjKrdvZuVC3","remoteAddr":"172.18.0.1","app":"PHP","message":"chmod(): No such file or directory at \/var\/www\/html\/lib\/private\/Config.php#233","level":3,"time":"2017-05-18T19:11:08+00:00","method":"GET","url":"\/","user":"--","version":""}" while reading response header from upstream, client: 172.18.0.1, server: , request: "GET / HTTP/1.1", upstream: "fastcgi://172.18.0.3:9000", host: "172.18.0.2"

The (well known blue) nextcloud page appears on the clients web browser but shows: `Can't write into config directory!

This can usually be fixed by giving the webserver write access to the config directory.`

This looks like wrong permissions inside the container. (I usually would spend more time with it but I think your docker experience is better than mine and I think others would also profit from this information)

Can you help me on that and extend the readme file or an examples documentation for the things which cannot be found in an existing manual (like setting correct env vars)

@topas-rec
Copy link

topas-rec commented May 18, 2017

I just thought that my error has to do with the missing groups setting on my docker host (I have to use sudo before every docker command) but I made the owncloud image work in a container and the nextcloud Apache image also works with these settings.

@topas-rec
Copy link

topas-rec commented May 18, 2017

Okay, I found this in the docker-compose file:

volumes: - ./nginx.conf:/etc/nginx/nginx.conf:ro
I assume 'ro' stands for read only. I wasn't able to test this. If you don't have other ideas I guess this might cause the issue. If so - why am I the only one having this issue?

I'll test this as soon as possible.

@topas-rec
Copy link

Another possible cause: greyltc/docker-owncloud#74

@topas-rec
Copy link

I've tested the :Z option at the config directory. That doesn't help.
I've also tried to remove the :ro option without success.
Can someone give me an idea where to continue troubleshooting? Should I create an issue because this is not directly related to the readme update?

@SnowMB
Copy link
Contributor Author

SnowMB commented May 20, 2017

Hey,

thanks for your feedback!

 I didn't know that I have (or am allowed) to change the docker compose file.

A compose file is a very specialized description of your services. It indeed has to be adapted to your needs.

 I didn't know that I have to set passwords of the db container.

Hm sorry, but I thought that's obvious 😁. I might add a line.

 I didn't knew what to download from the .example directory. I took all files. I started sudo docker-compose up (sudo because I didn't setup groups for docker - I'm personally fine with using sudo)

I think we'll have to structure the examples directory. If noone else does it I'll have a look when we finished the new Readme 😄.

 I wasn't able to set the environmental variable DOMAIN from the docker compose file in the docker command so I changed it in the docker compose file. (I didn''t knew this)

I don't know what variable you mean. There's just a VIRTUAL_HOST for the proxy and a LETSENCRYPT_HOST and LETSENCRYPT_EMAIL for letsencrypt. There is good documentation in the respective repos proxy and letsencrypt.

 I had to figure out myself that I need to set a valid domain name - localhost didn't worked for me.
 I didn't knew that a valid email is necessary for lets encrypt

This both has to do with the way letsencrypt and the reverse proxy work. As I said, these projects already have a good documentation, so I don't think it is necessary for us to give more information on that. We might add links to the repos tho.

#To your error:
You have a permission error on the /var/www/html/config directory.
It does not seem to be related to the nginx.conf file. So your tries to fix this where at the wrong place.

Please open an issue to get more help. This PR is just about the rework of the Readme and we would like to avoid any clutter that's unrelated to the topic. 👍

@topas-rec
Copy link

Hello,

Links to the documentation would be nice and I think helpful.
I'll open an issue for the permission problem.

Thanks and regards

@tilosp
Copy link
Member

tilosp commented May 23, 2017

@SnowMB is this PR ready for merge

@SnowMB
Copy link
Contributor Author

SnowMB commented May 23, 2017

Yeah should be. For now I think it's ok. Further improvements can be be done in new PRs. People are already confused about the old Readme. Will you do the merge?

@tilosp tilosp merged commit f8a4572 into nextcloud:master May 24, 2017
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.

4 participants