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.

Sign up for GitHub

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

Jump to bottom

Dockerfile small refactor #27757

Merged
merged 14 commits into from
Oct 29, 2023
Merged

Dockerfile small refactor #27757

merged 14 commits into from
Oct 29, 2023

Conversation

nfsec
Copy link
Contributor

@nfsec nfsec commented Oct 23, 2023 •
edited by wxiaoguang
Loading

  • Size and layer optimization,
  • Maintaining consistency in definitions (comments, apk etc.),

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 23, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 23, 2023
silverwind
silverwind reviewed Oct 23, 2023
View reviewed changes
Dockerfile Show resolved Hide resolved
@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 Oct 24, 2023
@silverwind silverwind added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Oct 24, 2023
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 25, 2023 •
edited
Loading

Why rename "docker/root/usr/bin/entrypoint → docker/root/usr/bin/entrypoint.sh" ?

If it is not a must, I would prefer to keep the old name to avoid breaking.

"Shebang compatible extension for entrypoint (like in rootless)." I do not see it as a must, because naturely, the "rootless" is totally different from the "root" image, they can't be switched.

wxiaoguang
wxiaoguang reviewed Oct 25, 2023
View reviewed changes
Dockerfile Outdated
Comment on lines 74 to 80
RUN chmod 755 /usr/bin/entrypoint.sh \
/app/gitea/gitea \
/usr/local/bin/gitea \
/usr/local/bin/environment-to-ini \
/etc/s6/gitea/* \
/etc/s6/openssh/* \
/etc/s6/.s6-svscan/*
Copy link
Contributor

@wxiaoguang wxiaoguang Oct 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For "Size and layer optimization":

IIRC there is a bug in the chmod step.

For example:

The problem is the RUN /bin/sh -c chmod 755 layer introduces another 44M change in "latest" image.

I think we should do chmod for the built binaries in the build-env stage, then the chmod layer won't cause so much a big change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved chmod to build-env.

@nfsec nfsec marked this pull request as draft October 25, 2023 18:29
@nfsec nfsec marked this pull request as ready for review October 25, 2023 20:45
@nfsec
Copy link
Contributor Author

nfsec commented Oct 25, 2023

Why rename "docker/root/usr/bin/entrypoint → docker/root/usr/bin/entrypoint.sh" ?

If it is not a must, I would prefer to keep the old name to avoid breaking.

"Shebang compatible extension for entrypoint (like in rootless)." I do not see it as a must, because naturely, the "rootless" is totally different from the "root" image, they can't be switched.

Why rename "docker/root/usr/bin/entrypoint → docker/root/usr/bin/entrypoint.sh" ?

If it is not a must, I would prefer to keep the old name to avoid breaking.

"Shebang compatible extension for entrypoint (like in rootless)." I do not see it as a must, because naturely, the "rootless" is totally different from the "root" image, they can't be switched.

Okey. Switched back to none-file-extension.

@nfsec nfsec requested a review from wxiaoguang October 25, 2023 20:49
wxiaoguang
wxiaoguang approved these changes Oct 26, 2023
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

Overall LGTM (not tested yet).

Leave it some days for more reviews & feedbacks.

@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 Oct 26, 2023
@wxiaoguang wxiaoguang added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 28, 2023
@wxiaoguang
Copy link
Contributor

If no more change request, wait to merge.

@wxiaoguang wxiaoguang added this to the 1.22.0 milestone Oct 28, 2023
@wxiaoguang wxiaoguang added the backport/v1.21 This PR should be backported to Gitea 1.21 label Oct 28, 2023
@lunny lunny merged commit 319414d into go-gitea:main Oct 29, 2023
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Oct 29, 2023
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Oct 29, 2023
@nfsec @GiteaBot
Dockerfile small refactor (go-gitea#27757)
e333fa6 
- Size and layer optimization,
- Maintaining consistency in definitions (comments, apk etc.),
@GiteaBot GiteaBot mentioned this pull request Oct 29, 2023
Merged
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Oct 29, 2023
KN4CK3R pushed a commit that referenced this pull request Oct 29, 2023
@GiteaBot @nfsec
Dockerfile small refactor (#27757) (#27826)
991c959 
Backport #27757 by @nfsec

- Size and layer optimization,
- Maintaining consistency in definitions (comments, apk etc.),

Co-authored-by: Patryk Krawaczyński <nfsec@users.noreply.github.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Oct 30, 2023
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
5506aae 
* giteaofficial/main:
  List all Debian package versions in `Packages` (go-gitea#27786)
  Fix merge base commit for fast-forwarded GitLab PRs (go-gitea#27825)
  Fix bad method call when deleting user secrets via API (go-gitea#27829)
  Change katex limits (go-gitea#27823)
  Dockerfile small refactor (go-gitea#27757)
  Use GitLab's squash_commit_sha when available (go-gitea#27824)
  [skip ci] Updated translations via Crowdin
  Upgrade xorm to 1.3.4 (go-gitea#27807)
fuxiaohei pushed a commit to fuxiaohei/gitea that referenced this pull request Jan 17, 2024
@nfsec @fuxiaohei
Dockerfile small refactor (go-gitea#27757)
e8a1199 
- Size and layer optimization,
- Maintaining consistency in definitions (comments, apk etc.),
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jan 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@silverwind silverwind silverwind approved these changes

Assignees
No one assigned
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/internal size/M Denotes a PR that changes 30-99 lines, ignoring generated files. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.
5 participants
@nfsec @wxiaoguang @silverwind @lunny @GiteaBot