Base on official PHP 7.0 Alpine image #21

kusmierz · 2016-04-10T21:57:07Z

see #16 and #20

mikehaertl · 2016-04-11T06:59:19Z

Dockerfile

- && ln -s ../../upload_large_dumps.ini /etc/php5/cli/conf.d
+## Add Tini
+RUN apk add --no-cache --repository http://dl-cdn.alpinelinux.org/alpine/edge/community/ tini
+ENTRYPOINT ["/usr/bin/tini", "--"]


@kusmierz Why do you add a init system? Why not rely on the default CMD from the official php-7 image?

Never mind, i found the related discussion and the reference to #17

…r rebuild, when new adminer version will arrive).

mathroc · 2016-04-11T07:20:38Z

Dockerfile

 ## Add the files
 ADD php.ini /usr/local/etc/php/conf.d/php.ini

 ## install adminer and default theme
 ENV ADMINER_VERSION 4.2.4

 RUN mkdir -p /var/www
-ADD https://www.adminer.org/static/download/$ADMINER_VERSION/adminer-$ADMINER_VERSION.php /var/www/index.php
+ADD http://www.adminer.org/static/download/$ADMINER_VERSION/adminer-$ADMINER_VERSION.php /var/www/index.php


why the change to https ?

By mistake, I've reverted it in e6f58f2

mathroc · 2016-04-11T07:21:01Z

seems good to me! 👍

kusmierz · 2016-04-11T07:24:36Z

I just thinking about difference between:

ADD http://www.adminer.org/static/download/$ADMINER_VERSION/adminer-$ADMINER_VERSION.php /var/www/index.php

and downloading it using wget (system one or from apk, but system's is without https support).

Mainly first one will download each and every time on build, and wget will probably use cached version. @mathroc what do you think?

kusmierz · 2016-04-11T07:25:33Z

php.ini

@@ -0,0 +1,4 @@
+upload_max_filesize = 2000M
+post_max_size = 2000M
+memory_limit = -1


Is it good idea to set here unlimited execution time and no memory limit?

exports & imports can take a very long time, not sure what would be enough and reasonnable

👍 Afaict these have only been moved (see also #8 and #4).

mathroc · 2016-04-11T07:29:43Z

@kusmierz if ADD works I'm fine with it. (dont' mind either way)

Always download latest version of adminer.

mikehaertl · 2016-04-11T10:04:14Z

Dockerfile

-EXPOSE 80
+## install adminer and default theme
+RUN mkdir -p /var/www
+ADD http://www.adminer.org/latest.php /var/www/index.php


We should create tags for this image, that are in sync with adminer (e.g. clue/adminer:4.2.4). So instead of latest we should point to the specific version https://www.adminer.org/static/download/4.2.4/adminer-4.2.4.php

I think so too, but as long as we have no tags here, I think it would be better to have latest.

Anyway, we could have tags like apache, alpine-php56, alpine-php7, etc. But do you think is worth for?

I don't think we need different variants (alpine, php56, ...). But I do think, we should tag the releases here, starting now. That's why I'm still for changing the URL.

@clue what do you think?

But I do think, we should tag the releases here, starting now.

See also #6 and #5. I think this is a bad fit for this PR, but I'm open to discussing this. So I'd vote for removing this from this PR for now in order to ease the discussion 👍

In other words: The current changeset LGTM 👍

clue · 2016-04-11T10:23:27Z

Dockerfile


-ADD supervisord.conf /etc/supervisor/conf.d/supervisord.conf
-CMD supervisord -c /etc/supervisor/conf.d/supervisord.conf
+CMD php -S 0.0.0.0:80 -t /var/www/


Do we really want to use PHP's built-in webserver here? (open for discussion)

I thought about this for some time too. @mathroc proposed this solution in #20.

I even made Dockerfile with nginx and php-fpm before, but I think @mathroc is right (but only in this particular case, see good explanation in smebberson/docker-alpine/alpine-nginx, "Aren't you only supposed to run one process per container?").

But, as we think about adminer - is dev tool, obviously. When we add nginx (at least 2 processes) and php-fpm (at least another 2) + supervisor or even smaller s6 instead only one (php's built-in webserver) it becomes a little bit overcomplicated.

+1 for @mathroc solution here.

I think you're making some very valid points here 👍

However, I'm not sure this should be discussed as part of this PR. I understand where you're coming from, but this particular statement is very well worth discussing IMHO:

But, as we think about adminer - is dev tool, obviously

Would you care to file a separate issue for this? 👍
As an alternative, we could also discuss this in a separate PR, as we can also change to this in our current image without switching to Alpine first.

ping, any thoughts on this, anybody?

IMO this change alone already warrants a separate PR / discussion.

There was PR/issue about it, created by @mathroc #20.

mathroc · 2016-04-11T11:42:46Z

@clue about your [comment on versionning](https://github.com/clue/docker-adminer/pull/19#discussion_r59180165], with the docker hub we can provide both the latest and tagged release. in other images I'm doing it this way:

master branch have SOMETHING_VERSION=latest and configured on docker hub to auto build as tag latest
releases branch with fixed version and tags Vx.x.x and tags are set to build automatically on docker hub too

anyway, this can be done in a second time, @kusmierz what do you think about sticking with latest for the time being and working on tags later ?

kusmierz · 2016-04-11T12:30:49Z

👍

kusmierz · 2016-04-18T11:33:18Z

@mathroc any chances to push this version into the wild?

mathroc · 2016-04-18T11:41:06Z

I don't have more comment for this PR. it's ready as far as I'm concerned. it's up to @clue to either merge this into master or in another branch (to tag it alpine un docker hub until it's a bit more tested)

mikehaertl · 2016-05-04T07:39:14Z

@mathroc @kusmierz I think it's not helpful for @clue to have 3 open PRs here now. Could you maybe close the irrelevant ones? Would be great to finally see some progress here.

kusmierz · 2016-05-30T21:09:12Z

Dockerfile

+ADD php.ini /usr/local/etc/php/conf.d/php.ini
+
+## install adminer and default theme
+ENV ADMINER_VERSION 4.2.4


Is it better download "latest" or having const with version?

@kusmierz See the discussion here: #5

Great, thanks @mikehaertl I knew I've seen it somewhere :)

There is some discussion about this in #5, however until we get to solve this I would vote to not introduce any additional changes in this PR 👍 (In other words, revert this change and)

After all, this PR ought to be about Alpine, right? :)

Tini has been relocated to /sbin/tini. Please update your scripts to use /sbin/tini going forward. /usr/bin/tini has been preserved for backwards compatibility in Alpine 3.4, but WILL BE REMOVED in Alpine 3.5.

mathroc · 2016-06-07T15:32:26Z

@clue do you think it can be merged once rebased (/ping @kusmierz ) ? or do you have other suggestions ?

clue · 2016-06-07T23:20:06Z

@clue do you think it can be merged once rebased

Afaict there are still some outstanding, uncommented issues in this PR, other than that I'm good with this change 👍

…adminer into feature/16-alpine-php70

mathroc · 2016-09-03T20:17:48Z

Dockerfile

@@ -1,35 +1,35 @@
-FROM ubuntu-debootstrap:14.04
+FROM php:7.0.7-alpine


maybe 7-alpine or at least 7.0-alpine ?

7-alpine is dangerous - why if they change something in ie. 7.2?

mathroc · 2016-09-03T20:20:53Z

Dockerfile

-WORKDIR /var/www
-RUN chown www-data:www-data -R /var/www
+## Add Tini
+RUN apk add --no-cache --repository http://dl-cdn.alpinelinux.org/alpine/edge/community/ tini


alpine version of official php images has been upgraded and it includes tini in the defauls packages list now. no need for the custom repository anymore (might even not work anymore)

Yeah, I've seen it before, but had no time to update it. Thanks.

mathroc · 2016-09-03T20:27:21Z

@kusmierz I've made a few more comments, do you mind taking a look and rebasing the PR ?

@clue is there still issues you'd like to be taken care for this PR? moving to php -S in another PR before this one maybe ?

kusmierz · 2016-09-05T09:42:52Z

@clue do we really need ODBC (#15)? It makes only problems (749192d)...

#16 php 7.0 support

970a70d

mikehaertl reviewed Apr 11, 2016
View reviewed changes

Little bit fixes (https and Tini is now higher, so it should be faste…

e6f58f2

…r rebuild, when new adminer version will arrive).

mathroc reviewed Apr 11, 2016
View reviewed changes

kusmierz reviewed Apr 11, 2016
View reviewed changes

kusmierz changed the title ~~#16 php 7.0 support~~ #16 alpine php 7.0 support Apr 11, 2016

kusmierz mentioned this pull request Apr 11, 2016

#16 alpine php 5.6 #19

Closed

mathroc mentioned this pull request Apr 11, 2016

use official php alpine image #20

Closed

6 tasks

Update Dockerfile

0fb8d51

Always download latest version of adminer.

mikehaertl reviewed Apr 11, 2016
View reviewed changes

kusmierz mentioned this pull request Apr 11, 2016

MS SQL Server support #7

Merged

clue reviewed Apr 11, 2016
View reviewed changes

clue changed the title ~~#16 alpine php 7.0 support~~ Base on official PHP 7.0 Alpine image May 4, 2016

Smaller image

dc87315

kusmierz reviewed May 30, 2016
View reviewed changes

Tini has been relocated to /sbin/tini.

8601a8a

Tini has been relocated to /sbin/tini. Please update your scripts to use /sbin/tini going forward. /usr/bin/tini has been preserved for backwards compatibility in Alpine 3.4, but WILL BE REMOVED in Alpine 3.5.

kusmierz added 2 commits June 8, 2016 11:07

#21 Downloading "latest" adminer instead tagged one (see #5)

48274b8

Merge branch 'feature/16-alpine-php70' of github.com:kusmierz/docker-…

7d58a98

…adminer into feature/16-alpine-php70

mathroc reviewed Sep 3, 2016
View reviewed changes

kusmierz added 2 commits September 5, 2016 09:51

Small fixes (@mathroc comments)

72ac62b

#15 #16 ODBC dirty fix

749192d

TimWolla mentioned this pull request Oct 13, 2016

Add Adminer official image docker-library/official-images#2252

Merged

9 tasks

kusmierz closed this by deleting the head repository Sep 23, 2024

		@@ -1,35 +1,35 @@
		FROM ubuntu-debootstrap:14.04
		FROM php:7.0.7-alpine

Base on official PHP 7.0 Alpine image #21

Base on official PHP 7.0 Alpine image #21

Conversation

kusmierz commented Apr 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathroc commented Apr 11, 2016

kusmierz commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathroc commented Apr 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clue Apr 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathroc commented Apr 11, 2016

kusmierz commented Apr 11, 2016

kusmierz commented Apr 18, 2016

mathroc commented Apr 18, 2016

mikehaertl commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathroc commented Jun 7, 2016

clue commented Jun 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kusmierz Sep 5, 2016 • edited Loading

Choose a reason for hiding this comment

mathroc commented Sep 3, 2016

kusmierz commented Sep 5, 2016

clue Apr 11, 2016 •

edited

Loading

kusmierz Sep 5, 2016 •

edited

Loading