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

gogs: avoid creating symlinks each run #30030

Merged
merged 2 commits into from Oct 2, 2017

Conversation

bobvanderlinden
Copy link
Member

@bobvanderlinden bobvanderlinden commented Oct 2, 2017

Motivation for this change

Currently gogs will create symlinks to its static data each run. It does so because there wasn't any way to define where gogs needed to find the static data and looked in its current directory.

Now STATIC_ROOT_PATH was introduced which allows setting the path where to find the static content. This PR makes use of that setting for the gogs module while also removing the creation of symlinks in the gogs wrapper.

Things done

Tested using: https://gist.github.com/bobvanderlinden/0c6914310f56a691cb12e00c8bca7059

Fixes #27866

@bobvanderlinden
Copy link
Member Author

bobvanderlinden commented Oct 2, 2017

I've cherry-picked this PR onto a local nixos-17.09 and it seems to be running correctly on my server.

@knedlsepp
Copy link
Member

While for NixOS this seems fine, I'm not sure how this plays out on non-NixOS machines. Before one could run gogs from any directory (with the downside of symlink-creation.) I think this might not work anymore after this change.

@orivej
Copy link
Contributor

orivej commented Oct 2, 2017

Thank you for figuring this out! But @knedlsepp is right, we should better patch gogs like this:

+++ pkg/setting/setting.go
- StaticRootPath = sec.Key("STATIC_ROOT_PATH").MustString(workDir)
+ StaticRootPath = sec.Key("STATIC_ROOT_PATH").MustString("@data@")

and then run

substituteInPlace pkg/setting/setting.go --subst-var data

@bobvanderlinden
Copy link
Member Author

Hmm, patching it in the package itself. That's indeed a nicer way to go for people running the package by itself. I'll try to get that into the PR.

@bobvanderlinden
Copy link
Member Author

@orivej I've added the patch and replaced @data@. Thanks for the suggestion. This works nicely in my tests.

@knedlsepp
Copy link
Member

Thank you guys for figuring this out. Looks like a good solution.

@orivej orivej merged commit 672bf13 into NixOS:master Oct 2, 2017
@orivej
Copy link
Contributor

orivej commented Oct 2, 2017

@bobvanderlinden Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants