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: Rework #14626

Closed
wants to merge 5 commits into from
Closed

Docker: Rework #14626

wants to merge 5 commits into from

Conversation

Kreyren
Copy link
Contributor

@Kreyren Kreyren commented Feb 10, 2021

!!! DO NOT MERGE !!!


I've reworked the docked backend, because i needed a more flexible development environment.

Do you want this in gitea? I would polish it more prior to merge and implement handling for other databases. ^-^

The important files are:

Blocked by: #14644 (This Merge Request will be finished after this one assuming that upstream wants it)

TODO

  • Upstream wants to merge this
  • Handling of user and group
    • Do a research to see whether we want to segregate the UNIX permission e.g. standalone group handling repositories
    • Build caching
      • User
        • User name
        • UID
        • Home location
      • Group
        • Group Name
        • GID
      • Add user to the group
    • Runtime configuration
      • User, change the build-configured X as needed
        • X=User name
        • X=UID
        • X=Home location
      • Group, change the build-configured X as needed
        • X=Group name
        • X=GID
    • Complies with expectations in Run gitea rootless with a custom user #14785
  • Resolve DNM tags
  • Peer-review by docker community
  • Automatization
    • Optimize Continuous integration (CI)
      • Hadolint
      • Shellcheck
    • Optimize Continuous Delivery (CD)
      • Publish docker container on merged update
    • Peer-review by DroneCI community
  • Update documentation
    • Consider blaming Golang devs to improve godoc inspired by rustdoc.
    • Proof-reading
  • Certificate of origin

Please check the following:

  1. Make sure you are targeting the master branch, pull requests on release branches are only allowed for bug fixes.

Done - Krey

  1. Read contributing guidelines: https://github.com/go-gitea/gitea/blob/master/CONTRIBUTING.md

To comply with https://github.com/go-gitea/gitea/blob/master/CONTRIBUTING.md#discuss-your-design:

This implement handling through variables allowing to change the file hierarchy and used configuration during a build time and on runtime through the use of docker-compose on which it does not have hard dependency (uses script that generates the config from variables that is called from docker-compose's command otherwise it uses gitea web)

It's designed to avoid maintaining two dockerfiles to make the maintenance more efficient by avoiding duplicate code through utilizing FHS3_0 service directory in alpine image to allow root and rootless run with option to run in a sandbox (e.g. chroot) within a docker environment.

To comply with https://github.com/go-gitea/gitea/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco:

Will be provided prior to non-draft merge request

  1. Describe what your pull request does and which issue you're targeting (if any)

Originally designed to test implementation to r e s o l v e #14562 intended to provide multiple services in a dockerfile to make sure that the implementation works with supported databases and to implement tests.

@kdumontnu
Copy link
Contributor

kdumontnu commented Feb 12, 2021

Thanks for contributing! +1 for shellcheck. Can you explain what you mean by wanting more flexibility?

I've been doing some work recently on my own docker environment. I'm quite happy with how it's ended up, so I thought I might share in case you find it useful.

Gist

I left all of the s6 stuff pretty much as is because I don't know much about it. Curious if you don't think it's useful.

Currently, I'm trying to make the same process work with rootless docker.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 12, 2021
@Kreyren
Copy link
Contributor Author

Kreyren commented Feb 12, 2021

Thanks for contributing! +1 for shellcheck. @kdumontnu

Also hadolint (https://github.com/hadolint/hadolint) which is fork of shellcheck designed to be used in dockerfiles.

Can you explain what you mean by wanting more flexibility? @kdumontnu

I find the current design too limiting as it makes it difficult to deploy multiple gitea instances with different configuration files and databases for testing purposes.

So this way it's generating the app.ini on runtime through the use of gitea-wrapper.sh only if the command is specified to use the wrapper otherwise it calls the gitea executable.

Note that currently this design is not meant to be used for production as it's depending on COPY . $GOPATH/code.gitea.... I want to do more research on that after #14644 is merged so that it can either use the local source code or fetch the remote where the idea is just giving people docker-compose.yml file to run docker-compose up and get gitea deployed.

I also find it too limiting to have it depending on volumes so i've designed it to use FHS3_0 service directory (/srv/gitea) that allows for root/rootless without the need for second dockerfile and if desired the service operators can define volumes through docker-compose.


Looking at the provided gist

This takes any environment variables in the form: GITEA__section__KEY and sets them in app.ini - super helpful!

I've done this through the shell wrapper that is using GITEA_SECTION_KEY as well, the difference that i see is to also define file hierarchy and user/group which i found helpful for my development.


About the env-file i found that too limiting as it doesn't support string replacement with default value:

# Sets variable kreyren to store value 'default' unless kreyren is already set
export kreyren="${kreyren:-"default"}"

It seems that docker is using a simple python interpreter to set these variables that are then not available in the CMD scope thus the shell wrapper.

-f docker-compose.yml
-f docker-compose.staging.yml \

My design expects the runtime defined within the docker-compose.yml or if needed it can use docker run ... as well which was done to use one database for multiple services so that i don't have to create the user on deployment.

@Kreyren
Copy link
Contributor Author

Kreyren commented Feb 12, 2021

@kdumontnu Btw. i am happy to discuss my design decisions more if you want to adapt them for your design.

@martencassel
Copy link

martencassel commented Feb 24, 2021

I don't see that is possible to specify what user context to run gitea in (user/group/uid/gid) from environment variables.
You only have the option by rebuilding the image with your custom --build-arg (ARG in the dockerfile)
The default if not specified could be the normal non-root git user with the defaults.

@Kreyren
Copy link
Contributor Author

Kreyren commented Feb 24, 2021

I don't see that is possible to specify what user context to run gitea in (user/group/uid/gid). @martencassel

Data defined in https://github.com/go-gitea/gitea/pull/14626/files#diff-7056ac181b6ee63e9510d8b82a9be9d372e8d2d01e207f893d29bb184e4b133bR55 which will be processed further

Expected configuration is:
a) GITEA_USER="kreyren" docker-command-here
b) Adding the GITEA_USER in docker-compose.yml environment property.

You only have the option by rebuilding the image with your custom --build-arg. @martencassel

I plan to add a handling to make this changable on runtime through the provided and optional script.

Can we make it possible to start gitea by switching to a user from environment variables. For example like this: minio/minio@2a79ea0/dockerscripts/docker-entrypoint.sh#L100

I find that yes i want to do something like this, but i also want to cache the step during a build.


Note that the handling is not full implemented and that this was created to aid development of #14644

I will be reworking this whole merge request based on my finding from that development and feedback here.

Added your #14780 in the TODO list.

@martencassel
Copy link

martencassel commented Feb 24, 2021

#14785

@Kreyren
Copy link
Contributor Author

Kreyren commented Feb 24, 2021

#14785

my fault!

@pat-s
Copy link
Member

pat-s commented Apr 5, 2021

@Kreyren I haven't followed the full discussion but it seems this PR is not going to be merged, isn't it? Can we close here?

@Kreyren
Copy link
Contributor Author

Kreyren commented Apr 5, 2021

@pat-s Will be finished once i handle #14644 currently being in work in github.com/kreytea (doing more changes on it as i need these for a development of my project)

feel free to close if this is inconvinient in the merge tracker will submit non-draft in that case once it's ready.

@pat-s
Copy link
Member

pat-s commented Apr 5, 2021

I was just wondering since I haven't read whether the proposed changes will be accepted in the end and there are a lof of conflicts in the current PR :)

Just wanted to check base and clean up a bit - the decision whether to close is on you :)

@Kreyren Kreyren closed this Apr 5, 2021
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants