{"action":"created","issue":{"url":"https://api.github.com/repos/adelton/docker-freeipa/issues/77","repository_url":"https://api.github.com/repos/adelton/docker-freeipa","labels_url":"https://api.github.com/repos/adelton/docker-freeipa/issues/77/labels{/name}","comments_url":"https://api.github.com/repos/adelton/docker-freeipa/issues/77/comments","events_url":"https://api.github.com/repos/adelton/docker-freeipa/issues/77/events","html_url":"https://github.com/adelton/docker-freeipa/pull/77","id":157470291,"number":77,"title":"Dockerfile cleanup","user":{"login":"mmarzantowicz","id":904394,"avatar_url":"https://avatars.githubusercontent.com/u/904394?v=3","gravatar_id":"","url":"https://api.github.com/users/mmarzantowicz","html_url":"https://github.com/mmarzantowicz","followers_url":"https://api.github.com/users/mmarzantowicz/followers","following_url":"https://api.github.com/users/mmarzantowicz/following{/other_user}","gists_url":"https://api.github.com/users/mmarzantowicz/gists{/gist_id}","starred_url":"https://api.github.com/users/mmarzantowicz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mmarzantowicz/subscriptions","organizations_url":"https://api.github.com/users/mmarzantowicz/orgs","repos_url":"https://api.github.com/users/mmarzantowicz/repos","events_url":"https://api.github.com/users/mmarzantowicz/events{/privacy}","received_events_url":"https://api.github.com/users/mmarzantowicz/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2016-05-30T09:56:24Z","updated_at":"2016-05-30T13:43:33Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/adelton/docker-freeipa/pulls/77","html_url":"https://github.com/adelton/docker-freeipa/pull/77","diff_url":"https://github.com/adelton/docker-freeipa/pull/77.diff","patch_url":"https://github.com/adelton/docker-freeipa/pull/77.patch"},"body":"I'd like to propose some changes in `Dockerfile` which ami at reducing overall number of layers the resulting image consists of and to enhance readability and maintainability of `Dockerfile`.\r\n\r\nShort summary of proposed changes:\r\n\r\n- Change ordering of instructions: e.g. `ENV` could go as close as possible to the top.\r\n- Use different, more dockerish way of formatting of `dnf` command.\r\n- I also removed `mkdir /run/lock` because it doesn't seem to be necessary any more (BTW it should be part of core file system if anything).\r\n- I also replaced `sed -i 's/^UUID=/# UUID=/' /etc/fstab` with more simplistic `: > /etc/fstab` since we really doesn't care about content of `/etc/fstab`.\r\n- I changed the way of how the content of `exit-via-chroot.conf` is generated: simply `COPY` existing file when image is built.\r\n\r\nI'd like to propose more changes to `Dockerfile` and the other files in this project if you find this interesting."},"comment":{"url":"https://api.github.com/repos/adelton/docker-freeipa/issues/comments/222495571","html_url":"https://github.com/adelton/docker-freeipa/pull/77#issuecomment-222495571","issue_url":"https://api.github.com/repos/adelton/docker-freeipa/issues/77","id":222495571,"user":{"login":"mmarzantowicz","id":904394,"avatar_url":"https://avatars.githubusercontent.com/u/904394?v=3","gravatar_id":"","url":"https://api.github.com/users/mmarzantowicz","html_url":"https://github.com/mmarzantowicz","followers_url":"https://api.github.com/users/mmarzantowicz/followers","following_url":"https://api.github.com/users/mmarzantowicz/following{/other_user}","gists_url":"https://api.github.com/users/mmarzantowicz/gists{/gist_id}","starred_url":"https://api.github.com/users/mmarzantowicz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mmarzantowicz/subscriptions","organizations_url":"https://api.github.com/users/mmarzantowicz/orgs","repos_url":"https://api.github.com/users/mmarzantowicz/repos","events_url":"https://api.github.com/users/mmarzantowicz/events{/privacy}","received_events_url":"https://api.github.com/users/mmarzantowicz/received_events","type":"User","site_admin":false},"created_at":"2016-05-30T13:43:33Z","updated_at":"2016-05-30T13:43:33Z","body":"1. Number of layers is not a problem but it makes building of images last longer. I know it's more a docker issue but there are no chances of any change here so we can reduce number of layers and as a result make build faster.\r\n\r\n1. Maybe I wasn't precise about `dnf/yum` order. I don't bother about ordering of packages but alphabetical order is the one I'm using when have no better alternative.  But I'd strongly recommend line breaks which really is more readable.\r\n\r\n1. I added `findutils` explicitly to the list because it is required in further instructions in `Dockerfile` (`find` and `xargs` commands). It's not present in core image and currently it's pulled as dependency by whatever package. I see it beneficial to have it on the list in case some rpm deps regarding `findutils` change in the future.\r\n\r\n1. I proposed `: > /etc/fstab` because it's more \"elegant\" variation of `> /etc/fstab` which has one big advantage over `sed` or `cat` - it doesn't create subshell and it has no external dependencies which is good to have inside `Dockerfile`. I have more patches to eliminate use of `cat` command to do something not related to file concatenation. I think it would be hard to miss when upstream remove `/etc/fstab` form image and it won't be anytime soon."}}
{"actor":{"display_login":"mmarzantowicz"}}