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

Make CustomPath, CustomConf and AppWorkPath configurable at build #6631

Open
wants to merge 23 commits into
base: master
from

Conversation

7 participants
@zeripath
Copy link
Contributor

commented Apr 15, 2019

The requirement to set environment variables can make it difficult to run gitea in non-development environments.

Using an LDFLAGS with our Makefile or by directly passing in -X variables to go build it should be possible to change the default locations thus making the environment variables unnecessary.

An example of this would be to say set -X "code.gitea.io/gitea/modules/setting.CustomPath=/data/gitea" for the docker, along with appropriate settings for CustomConf and AppWorkPath to remove the necessity to wrap the gitea binary in a shell script.

This would help our downstream packagers as they could make Gitea comply with the FHS and the equivalent debian hierarchy.

Signed-off-by: Andrew Thornton art27@cantab.net

Edit: It requires the full "code.gitea.io/gitea/modules/setting" import path.

Make CustomPath, CustomConf and AppWorkPath configurable at build
Signed-off-by: Andrew Thornton <art27@cantab.net>

@zeripath zeripath added the kind/build label Apr 15, 2019

@zeripath zeripath added this to the 1.9.0 milestone Apr 15, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

I've set backport 1.8 as I think this is a relatively minor change that could help users.

I'd change the docker build infrastructure to show how easy this makes things but I'm not certain where it is being built.

@codecov-io

This comment has been minimized.

Copy link

commented Apr 15, 2019

Codecov Report

Merging #6631 into master will decrease coverage by 0.01%.
The diff coverage is 31.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6631      +/-   ##
==========================================
- Coverage   40.73%   40.72%   -0.02%     
==========================================
  Files         421      421              
  Lines       57872    57881       +9     
==========================================
- Hits        23576    23572       -4     
- Misses      31168    31179      +11     
- Partials     3128     3130       +2
Impacted Files Coverage Δ
modules/setting/setting.go 47.38% <31.57%> (-0.79%) ⬇️
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️
routers/repo/view.go 41.6% <0%> (-1.01%) ⬇️
models/gpg_key.go 54.72% <0%> (+0.83%) ⬆️

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 45fa5cc...7e66235. Read the comment docs.

@mckaygerhard

This comment has been minimized.

Copy link

commented Apr 15, 2019

from now on it would be a good idea for each change, also document that change in the docs directory ... why is this change not documented in the docs directory installation files? @zeripath ? i mean, each coder could also write on each pul request respective documentation about their respective feature/change/bugfix if change some behaviour!

that was posted several times in forums that gitea lacks of proper documentation!

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

@mckaygerhard are you aware of how rude that comment reads?

This PR, although it could be merged, isn't really complete as I'd like - personally I'd prefer to get the docker build to move to it - which would be a good way to test and try to come up with documentation for this.

Do you have any suggestions for how this could be documented?

@mckaygerhard

This comment has been minimized.

Copy link

commented Apr 15, 2019

umm ok @zeripath what I wanted to say is that "it would be good", not that you should do it right now, I mean, as I mentioned :

it would be a good idea

zeripath added some commits Apr 15, 2019

Attempt to make the docker use the CustomPath functionality
Signed-off-by: Andrew Thornton <art27@cantab.net>
Ignore the new gitea-docker binary
Signed-off-by: Andrew Thornton <art27@cantab.net>
Adjust CustomConf setting to make them more friendly
Signed-off-by: Andrew Thornton <art27@cantab.net>
Add comment on SetCustomPath
Signed-off-by: Andrew Thornton <art27@cantab.net>
Add documentation
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2019

OK, We now build a special version of gitea for the docker with its default CustomPath in /data/gitea.

I've added the --custom-path option to the cmdline program.

Running gitea help will print out your computed configuration for the several of the variables.

Documentation is complete.

Show resolved Hide resolved cmd/web.go

zeripath added some commits Apr 16, 2019

make -c and -C work globally and update the documentation
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

Fixes #2228. Relevant to #480 and #2229.

@sapk
Copy link
Member

left a comment

Just reviewing the docker part, this need changes to make it work.

More over, I don't think we need a special binary for the docker image as it doesn't add anything. I understand the need for packing the be able to have a binary adjusted to the distrib paths but this is not the target for docker where we manage how we adjsut it. I will not block on this part.

Show resolved Hide resolved Dockerfile Outdated
Show resolved Hide resolved docker/Makefile Outdated

sapk and others added some commits Apr 16, 2019

Remove change to docker/Makefile
Co-Authored-By: zeripath <art27@cantab.net>
Missed one
Signed-off-by: Andrew Thornton <art27@cantab.net>
@sapk
Copy link
Member

left a comment

Thanks @zeripath. The parts that I reviewed looks good now (they are removed now so perfect 😄)
The PR looks good but I haven't tested it so this comment is just to validate my previous requests.

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2019

I do think it would be easier if the gitea installed on the docker was just built to work with it's paths correct for the default docker installation without environment variables or options etc. but it could be done at a later date.

@sapk

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

@zeripath the current main problem with the current docker installation is that it use the same path for GITEA_CUSTOM and for app_work_dir it is confusing and should be fix but will break some installation. In general, docker container are configurate by ENV so I don't see the need to remove one set by default.
Reviewing your PR, I found that GITEA_CUSTOM is enforce at various point where it shoudln't : https://github.com/go-gitea/gitea/blob/master/docker/etc/profile.d/gitea.sh
and not forwarded at other:
https://github.com/go-gitea/gitea/blob/master/docker/etc/s6/gitea/setup#L9
This cleanup could be done in this PR but I consider that it should be tackle separatly as it is a wider problem. I will open a PR to tackle that and make the app_work_dir distinguisable.

edit: We are also using the same path for APP_DATA_PATH

@lafriks

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Should we really backport this to 1.8?

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

When it was simply a two liner I thought so. Now it's a bit more involved I'm less definite.

In terms of why backport:

I think downstream packers might like this functionality - it does finally actually allow Gitea to be built in a way that complies with the filesystem hierarchy standard and should remove some gotchas with running the Gitea subcommands in those environments.

It's also helpful for those on the default build as you can now see where several paths are set through gitea help

I'm not stuck either way and whilst I understand the frustration of our Debian users that the Gitea binary uses nonstandard paths - I don't understand why they can't just shadow the binary with a compliant script, albeit this script would be complicated without this PR due to the difficulty of setting -c. This does however remove the necessity of even having a script.

@lafriks lafriks removed the backport/v1.8 label Apr 18, 2019

@lafriks

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

I will remove backporting label as it is more of a feature than bugfix, anyway creating correct app.ini and using -c this already can be achieved so it is not so important if it will be only from 1.9

@zeripath

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

No problems

zeripath added some commits Apr 19, 2019

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Apr 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.