deis / postgres

A PostgreSQL database used by Deis Workflow.
https://deis.com
MIT License
36 stars 22 forks source link

fix(Dockerfile): convert tabs to spaces #151

Closed bacongobbler closed 8 years ago

bacongobbler commented 8 years ago

This commit converts all spacing to tabs and prefixes concatenated strings with '&&' rather than suffixing the strings. This brings a more consistent style to the Dockerfile.

deis-bot commented 8 years ago

@helgi, @krancour and @mboersma are potential reviewers of this pull request based on my analysis of git blame information. Thanks @bacongobbler!

helgi commented 8 years ago

I'm thumbs down on this since all other repos are && at the end and use spaces.

bacongobbler commented 8 years ago

We used this syntax before #145.

Some popular community Docker images also use this style when chaining commands:

https://github.com/docker/docker/blob/master/Dockerfile#L89-L95 https://github.com/docker-library/postgres/blob/master/9.4/Dockerfile#L9-L19 https://github.com/docker-library/elasticsearch/blob/master/2.2/Dockerfile#L24-L27

Personally I'd rather follow community's best practices. It does look subjectively cleaner, in my opinion.

bacongobbler commented 8 years ago

As for spaces vs. tabs, same argument applies here. If we want to follow a consistent style then it's probably best to follow Docker's best practices, which in that case would be prefixed ampersands and tabs.

helgi commented 8 years ago

Well if you update this one then please update all other images. I'm not going to try to argue it against this change. I find the current style cleaner. A few community images isn't any more of a standard than if I pointed to numerous other Dockerfiles or shell scripts :)

bacongobbler commented 8 years ago

Sure, I'd be happy to go on a formatting crusade. I'll keep this out until I do the same to the rest of the Dockerfiles.

bacongobbler commented 8 years ago

And for what it's worth, it's not explicitly stated but https://docs.docker.com/engine/userguide/eng-image/dockerfile_best-practices/#/run does demonstrate this syntax as well. Nothing is said about tabs/spaces so I can change that back if that's preferred. Just want to make it one or the other rather than mixing the two.

helgi commented 8 years ago

Pick a style you want and go with that if you are going over it all anyway

mboersma commented 8 years ago

Tabs cause more problems than they solve IMHO and I also find the suffixed && style more readable. I also think we are inferring a best practice that isn't that pervasive, although I could be persuaded otherwise.

Having said that, I would like to see consistency among our Dockerfiles if possible, so if we can agree here we should do it everywhere. :hobgoblin:

bacongobbler commented 8 years ago

Alright if the rest of our components were using the suffixed style, let's use that. I've changed this PR to use spaces so I'll edit the PR to reflect that, and revert the changes to the poor ampersands

krancour commented 8 years ago

Fwiw, the && at the end of the preceding lines vs the start of successive lines might have came mostly from me back when I was heavily refactoring all our components' Dockerfiles to transition from Alpine to Ubuntu slim, at which time I also made other optimizations to reduce image size (e.g. combining successive RUN directives with && \ to reduce the number of layers).

I'm glad @mboersma likes it the way I did it, but personally, and with hindsight being 20/20, I actually agree with @bacongobbler and wish I'd put the && at the beginning of lines as I now find that more readable. (The way my brain works changed? idk.)

All that being said, even being as big a fan of consistency as I am, it's probably not useful to obsess over keeping this (or tabs vs spaces) consistent from repo to repo. As long as we're consistent within a single Dockerfile, I think that's good enough. i.e. I wouldn't recommend the aforementioned formatting crusade. </opinion>