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

Gitea Snap update issue #16209 #17157

Closed
wants to merge 8 commits into from
Closed

Conversation

OleHusgaard
Copy link

Fixes #16209 for new repositories created. The problem is that hooks in new repositories created when running in snap include the snap revision in the path. As the snap is refreshed the hooks eventually break.
This pull request does not fix repositories already created. Here a good workaround has been suggested by JensTheCoder.
Please be aware that I am a newbie to both gitea, snap and golang.

@codecov-commenter
Copy link

codecov-commenter commented Sep 26, 2021

Codecov Report

Merging #17157 (2df0e4c) into main (f2e7d54) will decrease coverage by 0.01%.
The diff coverage is 55.10%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #17157      +/-   ##
==========================================
- Coverage   45.31%   45.30%   -0.02%     
==========================================
  Files         773      774       +1     
  Lines       86873    87056     +183     
==========================================
+ Hits        39366    39440      +74     
- Misses      41151    41230      +79     
- Partials     6356     6386      +30     
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
models/action.go 48.86% <0.00%> (ø)
models/avatar.go 35.38% <0.00%> (ø)
models/error.go 39.83% <ø> (+0.29%) ⬆️
models/fixture_generation.go 70.00% <0.00%> (ø)
models/gpg_key_verify.go 13.46% <0.00%> (ø)
models/issue_lock.go 0.00% <0.00%> (ø)
models/migrate.go 0.00% <0.00%> (ø)
models/pull_sign.go 37.64% <0.00%> (ø)
models/repo_archiver.go 6.25% <0.00%> (ø)
... and 204 more

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 74542ad...2df0e4c. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 26, 2021
return strings.ReplaceAll(appPath, "\\", "/"), err
appPath = strings.ReplaceAll(appPath, "\\", "/")

// Fix issue 16209
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you write more comments here? It will be easier for other people to understand what the issue is and how it is fixed.

Copy link
Author

Choose a reason for hiding this comment

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

I added comments about the problem, and how it is fixed,

Copy link
Contributor

@wxiaoguang wxiaoguang Sep 26, 2021

Choose a reason for hiding this comment

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

Thank you, it's clear for me. I think we should be more careful here:

  1. We should check whether the current path really exists and is valid. If not, an ERROR log should be reported, and fallback to use old AppPath.
  2. We should output an INFO level log to tell users that we are in SNAP and have changed the AppPath for this purpose. Or users may be confused by this behavior (it will be very hard to debug)

@techknowlogick
Copy link
Member

Here is the solution I was talking about in the issue ticket:

replace var appPath string with

appPath := AppPath

if appPath != "" {
	return appPath, nil
}

then put the following in the snapcraft yaml above the make build command

export LDFLAGS="-X \"code.gitea.io/gitea/modules/setting.AppPath=/var/snap/gitea/current/gitea\" ${LDFLAGS}"

(note: I'm not 100% sure the above filepath is correct, please ensure it is correct)

@wxiaoguang
Copy link
Contributor

then put the following in the snapcraft yaml above the make build command

export LDFLAGS="-X \"code.gitea.io/gitea/modules/setting.AppPath=/var/snap/gitea/current/gitea\" ${LDFLAGS}"

@techknowlogick It's not a good idea to hard code the path in source code when building, what if sys admin changed the snap path ... Can we find someway to pass the AppPath by environment variables provided by snap system?

I have no experience with snap, but I think there should be some way: https://snapcraft.io/docs/environment-variables , https://snapcraft.io/docs/parts-environment-variables .

@techknowlogick
Copy link
Member

@wxiaoguang yeah, we could do similarly to the getWorkPath function, where if os.LookupEnv("GITEA_WORK_DIR"); is set, then it'll return the value of that env var (except of course it wouldn't be the that exact env var).

@OleHusgaard
Copy link
Author

OleHusgaard commented Sep 27, 2021

@wxiaoguang yeah, we could do similarly to the getWorkPath function, where if os.LookupEnv("GITEA_WORK_DIR"); is set, then it'll return the value of that env var (except of course it wouldn't be the that exact env var).

@techknowlogick I like this approach, as it would be cleaner and simpler than my hack. However we still have to get the right path, which it does not look like snap is providing. But we could derive it from the other snap variables by adding something like this to the snap environment:
GITEA_APP_PATH: "$(echo "$SNAP/$SNAP_NAME"|sed "s/$SNAP_REVISION/current/")"

Should I try this approach instead?

@techknowlogick
Copy link
Member

@OleHusgaard oh yea! That's a great approach, thanks :)

@wxiaoguang
Copy link
Contributor

The problem should have been fixed by

@wxiaoguang wxiaoguang closed this Nov 18, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
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. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gitea Snap update issue
5 participants