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

Rework of Examples Section #125

Merged
merged 25 commits into from
Oct 7, 2017
Merged

Rework of Examples Section #125

merged 25 commits into from
Oct 7, 2017

Conversation

tilosp
Copy link
Member

@tilosp tilosp commented Jul 6, 2017

Closes #103 Closes #98 Closes #95 Closes #75 Closes #86 Closes #104

TODO:

@SnowMB
Copy link
Contributor

SnowMB commented Jul 11, 2017

Hey @tilosp,

thanks for starting to work on the examples section. Here a few thoughts on the folder structure.

current version

examples\apache\dockerfiles\Dockerfile.cron
examples\apache\dockerfiles\supervisord.conf
examples\fpm\dockerfiles\Dockerfile.cron
examples\fpm\dockerfiles\supervisord.conf

This has the disadvantage that additional files would clutter the structure and users might ask themself what are these files for (e.g. when s.o. wants to build an Dockerfile.imap and there is the supervisord.conf file). It could get worse when more examples are added.

I would prefer to differentiate between the base images (fpm/apache) in the last subfolder. That leaves three options:

option 1

examples\dockerfiles\cron\Dockerfile.apache
examples\dockerfiles\cron\Dockerfile.fpm
examples\dockerfiles\cron\supervisord.conf

This has the advantage, that the additional files would not be redundant. As long as the additional files can be used in all base images it would be a very clean folder structure.

option 2

examples\dockerfiles\cron\apache\Dockerfile
examples\dockerfiles\cron\apache\supervisord.conf
examples\dockerfiles\cron\fpm\Dockerfile
examples\dockerfiles\cron\fpm\supervisord.conf

Reintroduces redundency, but in cases where different additional files for the base images are needed, their would be no conflicts. Also it is possible to run the default commands docker build . or docker-compose up -d directly in the folder.

option 3

examples\dockerfiles\cron\apache\Dockerfile
examples\dockerfiles\cron\fpm\Dockerfile
examples\dockerfiles\cron\supervisord.conf

This last option could be used to avoid the redundency issue by moving matching files to the parent folder. But at the same time the Dockerfiles get dependant on these directories.

My opinion:

option 1 is useful, if someone dislikes using to many subfolders 😉
Personally I prefer option 2 because it seems to be the cleanest one.

@tilosp tilosp force-pushed the rework-examples branch from db3e8a8 to 80c7c34 Compare July 12, 2017 12:10
@tilosp
Copy link
Member Author

tilosp commented Jul 12, 2017

@SnowMB i agree with you that option 2 is the cleanest one and i am fine with using many sub-directories.
And by the way supervisord.conf is not redundant because one of them starts fpm and the other one apache.

@SnowMB
Copy link
Contributor

SnowMB commented Jul 12, 2017

You're right. Missed that. But you unterstood what I tried to explain 😁

@SnowMB SnowMB mentioned this pull request Jul 31, 2017
@@ -0,0 +1,12 @@
<?
$CONFIG = array (
'memcache.local' => '\\OC\\Memcache\\APCu',
Copy link
Member Author

Choose a reason for hiding this comment

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

@SnowMB this line is redundant, it is already part of the image apcu.config.php

Copy link
Contributor

Choose a reason for hiding this comment

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

done

@tilosp tilosp force-pushed the rework-examples branch 5 times, most recently from b73e9f7 to c7a0265 Compare August 10, 2017 17:06
@SnowMB
Copy link
Contributor

SnowMB commented Aug 14, 2017

Hey @tilosp,

I got a quick question:

You have an app folder for each example creating a derived image copying the *.php files. Wouldn't it be easier mounting them in the docker-compose.yml file and using the default image?

Are there any advantages / disadvantages that I miss?

@tilosp
Copy link
Member Author

tilosp commented Aug 14, 2017

@SnowMB
There is one advantage.
Mounting them is just fine when the docker engine is on the same machine as the docker client.
But if you use a docker engine on a remote machine mounting will fail because the files are only on the client side so that the remote docker engine can't mount them. This problem came up in #104.
When building an image the client transfers the build context (all file which are not excluded by a .dockerignore file) to the docker engine to build it there.

I forgot to do the same for the nginx.conf which I will do later.

@SnowMB
Copy link
Contributor

SnowMB commented Aug 14, 2017

Ah, I see. Great solution for that case! 👍

@SnowMB
Copy link
Contributor

SnowMB commented Aug 16, 2017

@tilosp I reworked the proxy examples to fit your style and added a readme file. Please review and maybe add a few words to the readme.

@tilosp tilosp force-pushed the rework-examples branch 3 times, most recently from 7127678 to dee6524 Compare August 28, 2017 16:15
@tilosp
Copy link
Member Author

tilosp commented Sep 24, 2017

@SnowMB please take a look.

I removed the example with collabora for now because it does not work. The problem is that the nginx proxy is not setup to handle WebSocket connection and collabora requires WebSocket connection. I would like to merge this PR without a collabora example, we can add one in a later PR.

But we should add a link to the main readme which links to the example readme. Maybe you have an idea where exactly this link should go.

I would also suggest a rework of README.md#running-this-image-with-docker-compose. Maybe by just replacing it with a link to the docker-compose examples

@tilosp
Copy link
Member Author

tilosp commented Sep 27, 2017

ping @SnowMB

Copy link
Contributor

@SnowMB SnowMB left a comment

Choose a reason for hiding this comment

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

The link in the main readme is easier to add once it is merged, because it should to point to the master branch. I think we can finish this pr now and do some fine tuning in additional pr/issues.

LGTM so far 👍


### insecure

To use this example complete the following steps:
Copy link
Contributor

Choose a reason for hiding this comment

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

add a sentence why this setup is insecure and that it should not be used in production.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@SnowMB SnowMB mentioned this pull request Oct 4, 2017
@tilosp tilosp requested a review from SnowMB October 5, 2017 14:18
Copy link
Contributor

@SnowMB SnowMB left a comment

Choose a reason for hiding this comment

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

lgtm

@tilosp tilosp merged commit fa3b83c into master Oct 7, 2017
@tilosp tilosp deleted the rework-examples branch October 7, 2017 20:30
tilosp added a commit to tilosp/docker-library-docs that referenced this pull request Oct 7, 2017
@tilosp tilosp changed the title [WIP] Rework of Examples Section Rework of Examples Section Oct 7, 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.

2 participants