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

[docker] drop the docker Makefile from the image #6507

Merged
merged 10 commits into from
May 6, 2019

Conversation

das7pad
Copy link
Contributor

@das7pad das7pad commented Apr 4, 2019

The - in front of the include statement causes make to ignore a missing file [1].

make docker will continue to work on the dev machine.

[1] https://www.gnu.org/software/make/manual/html_node/Include.html

Signed-off-by: Jakob Ackermann <das7pad@outlook.com>
@lafriks lafriks added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Apr 4, 2019
@sapk
Copy link
Member

sapk commented Apr 4, 2019

@das7pad What is the goal/use case ?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 4, 2019
@das7pad
Copy link
Contributor Author

das7pad commented Apr 4, 2019

There is no need to have a Makefile with docker commands in the docker container.

# docker run --rm -t --entrypoint=/bin/ls gitea/gitea:latest
Makefile  data      home      mnt       root      srv       usr
app       dev       lib       opt       run       sys       var
bin       etc       media     proc      sbin      tmp

# docker run --rm -t --entrypoint=/bin/ls gitea/gitea:no-make
app    data   etc    lib    mnt    proc   run    srv    tmp    var
bin    dev    home   media  opt    root   sbin   sys    usr

# docker images gitea/gitea
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
gitea/gitea         no-make             00d2a5c2ba6b        43 minutes ago      94.8MB
gitea/gitea         latest              f205743304c7        36 hours ago        94.9MB

@codecov-io
Copy link

codecov-io commented Apr 4, 2019

Codecov Report

Merging #6507 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6507      +/-   ##
==========================================
+ Coverage   41.13%   41.15%   +0.01%     
==========================================
  Files         425      425              
  Lines       58484    58484              
==========================================
+ Hits        24059    24067       +8     
+ Misses      31238    31231       -7     
+ Partials     3187     3186       -1
Impacted Files Coverage Δ
models/repo_list.go 67.89% <0%> (+1.05%) ⬆️
modules/log/event.go 65.98% <0%> (+1.52%) ⬆️
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0081cd8...5c22bf8. Read the comment docs.

@sapk
Copy link
Member

sapk commented Apr 4, 2019

@das7pad Sorry, I was thinking that we only copy subfolder usr and etc. It must have changed or was on my todo list and think it was allready done.

Copy link
Member

@sapk sapk left a comment

Choose a reason for hiding this comment

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

I am more in favor on only copying subfolder etc and usr than re-adding another file at root project folder. But still LGTM

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 4, 2019
@das7pad
Copy link
Contributor Author

das7pad commented Apr 6, 2019

I am fine with adding the two directories (/etc and /usr) explicit instead of re-adding the ignore file.

While preparing a patch I noticed that the Dockerfile commands COPY and ADD do not include the source directory. This line COPY docker/etc docker/usr / copies the contents of docker/etc and docker/usr into the root.
For example docker/etc/nsswitch.conf ends up as /nsswitch.conf.

I see two options at hand:

  • two layers for the copy calls
  • copy docker/etc and docker/usr into a new directory and copy this directory into the docker images root
    e.g. docker/etc -> docker/root/etc and the Dockerfile line COPY docker/root /

Let's see what the other maintainers think about it.

@lafriks
Copy link
Member

lafriks commented Apr 8, 2019

imho second option would be better

Signed-off-by: Jakob Ackermann <das7pad@outlook.com>
ref 32b3253

Signed-off-by: Jakob Ackermann <das7pad@outlook.com>
techknowlogick and others added 2 commits April 13, 2019 20:51
# Conflicts:
#	docker/root/etc/s6/syslogd/finish
#	docker/root/etc/s6/syslogd/run
#	docker/root/etc/s6/syslogd/setup
@lafriks lafriks added this to the 1.9.0 milestone Apr 17, 2019
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 17, 2019
@techknowlogick techknowlogick merged commit dab38c3 into go-gitea:master May 6, 2019
@das7pad das7pad deleted the misc/drop-makefile branch May 6, 2019 19:25
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants