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

Merged

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath 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.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Apr 15, 2019
@zeripath zeripath added this to the 1.9.0 milestone Apr 15, 2019
@zeripath
Copy link
Contributor Author

zeripath 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.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 15, 2019
@codecov-io
Copy link

codecov-io commented Apr 15, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@ccf4783). Click here to learn what that means.
The diff coverage is 31.57%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #6631   +/-   ##
=========================================
  Coverage          ?   41.22%           
=========================================
  Files             ?      421           
  Lines             ?    58064           
  Branches          ?        0           
=========================================
  Hits              ?    23934           
  Misses            ?    30961           
  Partials          ?     3169
Impacted Files Coverage Δ
modules/setting/setting.go 47.48% <31.57%> (ø)

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 ccf4783...2fff29e. Read the comment docs.

@mckaygerhard
Copy link

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
Copy link
Contributor Author

@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
Copy link

mckaygerhard 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
Copy link
Contributor Author

zeripath 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.

sapk and others added 3 commits April 16, 2019 14:32
Co-Authored-By: zeripath <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
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.

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
Copy link
Contributor Author

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
Copy link
Member

sapk 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
Copy link
Member

lafriks commented Apr 18, 2019

Should we really backport this to 1.8?

@zeripath
Copy link
Contributor Author

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
Copy link
Member

lafriks 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
Copy link
Contributor Author

No problems

@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 23, 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 29, 2019
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.

8 participants