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 AppDataPath absolute against the AppWorkPath if it is not #19815

Merged
merged 4 commits into from
Jun 6, 2022

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented May 26, 2022

There are multiple repeated issues whereby a non-absolute provided
APP_DATA_PATH causes strange issues.

This PR simply absolutes the APP_DATA_PATH against the AppWorkPath if
its not so. It also ensures that AppWorkPath is also always absolute.

Ref #19367

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

⚠️ BREAKING ⚠️

Server administrators should be aware that both the work path and the data path are now made absolute.

  • The work path will be made absolute against the PWD and if that is not possible against the AppPath
  • The APP_DATA_PATH will be made absolute against the work path.

This second change is a breaking change for administrators who had APP_DATA_PATH as a relative path and the work path was not the current PWD.

However, because of the multiple issues reported with non-absolute APP_DATA_PATHs we expect that this change will affect few people.

There are multiple repeated issues whereby a non-absolute provided
APP_DATA_PATH causes strange issues.

This PR simply absolutes the APP_DATA_PATH against the AppWorkPath if
its not so. It also ensures that AppWorkPath is also always absolute.

Ref go-gitea#19367

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added this to the 1.17.0 milestone May 26, 2022
@techknowlogick
Copy link
Member

Should we log a warn or info now we are ensuring absolute paths against workpath? Either way this LGTM

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 27, 2022
@silentcodeg
Copy link
Contributor

This is a breaking change for all existing Gitea installations that have a relative AppDataPath in app.ini that is not against AppWorkPath.

@techknowlogick techknowlogick added the pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! label May 27, 2022
@@ -478,6 +478,10 @@ func getWorkPath(appPath string) string {
workPath = appPath[:i]
}
}
workPath = strings.ReplaceAll(workPath, "\\", "/")
Copy link
Member

Choose a reason for hiding this comment

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

filepath.Join can deal with backslashes on Windows. It doesn't need such hacks. After all, backslash is a valid path character on many file sytems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to be consistent with the rest of the filepath munging in this file.

singuliere referenced this pull request in void-linux/void-packages May 28, 2022
- add log service
- cd into $HOME before running: fixes issues with APP_DATA_PATH being a
  relative path, which is the default in the example config. See [1]

  [1] go-gitea/gitea#19367
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

zeripath commented Jun 1, 2022

Should we log a warn or info now we are ensuring absolute paths against workpath? Either way this LGTM

Have done so.

@zeripath
Copy link
Contributor Author

zeripath commented Jun 1, 2022

This is a breaking change for all existing Gitea installations that have a relative AppDataPath in app.ini that is not against AppWorkPath.

I'm not sure that such configurations previously worked properly in any case.

I can add a breaking notice.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@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 Jun 6, 2022
@techknowlogick techknowlogick merged commit c48706e into go-gitea:main Jun 6, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jun 9, 2022
* giteaofficial/main:
  Prevent NPE whilst migrating if there is a team request review (go-gitea#19855)
  [skip ci] Updated translations via Crowdin
  Add support for rendering terminal output with colors (go-gitea#19497)
  Fix viewed images not loading in a PR (go-gitea#19919)
  Remove out-dated comments (go-gitea#19921)
  Automatically render wiki TOC (go-gitea#19873)
  Improve wording on delete access token modal (go-gitea#19909)
  [skip ci] Updated translations via Crowdin
  Add breaking email restrictions checker in doctor (go-gitea#19903)
  Ensure minimum mirror interval is reported on settings page (go-gitea#19895)
  Improve UX on modal for deleting an access token (go-gitea#19894)
  update discord invite (go-gitea#19907)
  Only log non ErrNotExist errors in git.GetNote  (go-gitea#19884)
  [skip ci] Updated translations via Crowdin
  Update frontend guideline (go-gitea#19901)
  Make AppDataPath absolute against the AppWorkPath if it is not (go-gitea#19815)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
…tea#19815)

* Make AppDataPath absolute against the AppWorkPath if it is not

There are multiple repeated issues whereby a non-absolute provided
APP_DATA_PATH causes strange issues.

This PR simply absolutes the APP_DATA_PATH against the AppWorkPath if
its not so. It also ensures that AppWorkPath is also always absolute.

Ref go-gitea#19367

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

* Add logging

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

* absolute workpath against pwd instead of app path first

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

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
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. pr/breaking Merging this PR means builds will break. Needs a description what exactly breaks, and how to fix it! type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants