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

Set session and indexers' data files rel to AppDataPath #2192

Merged
merged 3 commits into from
Nov 3, 2017

Conversation

dubeg
Copy link
Contributor

@dubeg dubeg commented Jul 21, 2017

The setting AppDataPath now defaults to being relative to the working directory.
The session svc's PROVIDER_CONFIG now defaults to AppDataPath/data/sessions.
The issue indexer's IssuePath now defaults to AppDataPath/indexers/issues.bleves.

This change targets #2159.

@cez81
Copy link
Contributor

cez81 commented Jul 21, 2017

Also fixes #1378

@@ -1243,7 +1243,7 @@ func newCacheService() {
func newSessionService() {
SessionConfig.Provider = Cfg.Section("session").Key("PROVIDER").In("memory",
[]string{"memory", "file", "redis", "mysql"})
SessionConfig.ProviderConfig = strings.Trim(Cfg.Section("session").Key("PROVIDER_CONFIG").String(), "\" ")
SessionConfig.ProviderConfig = strings.Trim(Cfg.Section("session").Key("PROVIDER_CONFIG").MustString(path.Join(AppDataPath, "data/sessions")), "\" ")
Copy link
Member

Choose a reason for hiding this comment

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

Same here

models/models.go Outdated
@@ -147,7 +147,7 @@ func LoadConfigs() {
DbCfg.Timeout = sec.Key("SQLITE_TIMEOUT").MustInt(500)

sec = setting.Cfg.Section("indexer")
setting.Indexer.IssuePath = sec.Key("ISSUE_INDEXER_PATH").MustString("indexers/issues.bleve")
setting.Indexer.IssuePath = sec.Key("ISSUE_INDEXER_PATH").MustString(path.Join(setting.AppDataPath, "indexers/issues.bleve"))
Copy link
Member

Choose a reason for hiding this comment

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

@lafriks lafriks added type/enhancement An improvement of existing functionality lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 21, 2017
@dubeg dubeg force-pushed the master branch 2 times, most recently from 9b9890a to ca3ff45 Compare July 24, 2017 21:36
@dubeg
Copy link
Contributor Author

dubeg commented Jul 24, 2017

I am not used to the pull request process. I amended my current commit to include the feedback. Is this the correct method?

I added a new setting variable containing the working directory named AppWorkPath. Maybe I should've named it AppWorkDir?

@lafriks
Copy link
Member

lafriks commented Jul 24, 2017

Now it is called differently in multiple places so either dir or path but do not mix (i would vote for path). You can add commits to your branch and they will show up here

@dubeg
Copy link
Contributor Author

dubeg commented Jul 25, 2017

Ok I changed to workPath the fn & vars that were named workDir. Anything else?

@dubeg
Copy link
Contributor Author

dubeg commented Jul 31, 2017

I just added a minor change to the path ProviderConfig. I forgot to remove the "data" in "data/sessions" when joining with AppDataPath.

By the way, I noticed we use path.Join as well as filepath.Join almost as often. I checked their definition and they're a bit different. What's the logic here? Should we use one or the other, or does it not matter?

@dubeg dubeg force-pushed the master branch 4 times, most recently from 21714a3 to 5c0f65f Compare August 8, 2017 22:01
@lafriks
Copy link
Member

lafriks commented Aug 17, 2017

@dubeg Please fix formatting issues (see drone build that is failing). path.Join should be used for URL's, filepath.Join for file path :)

@lunny
Copy link
Member

lunny commented Oct 31, 2017

@dubeg need rebase

@cez81 cez81 mentioned this pull request Oct 31, 2017
7 tasks
@@ -1188,7 +1187,7 @@ func NewXORMLogService(disableConsole bool) {
if err = os.MkdirAll(path.Dir(logPath), os.ModePerm); err != nil {
panic(err.Error())
}
logPath = filepath.Join(filepath.Dir(logPath), "xorm.log")
logPath = path.Join(filepath.Dir(logPath), "xorm.log")
Copy link
Member

Choose a reason for hiding this comment

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

Why change filepath to path?

@lunny
Copy link
Member

lunny commented Oct 31, 2017

need rebase.

@dubeg
Copy link
Contributor Author

dubeg commented Oct 31, 2017

Sorry I've been terribly busy and also lazy since August. I'll try to fix the PR this week. As for filepath to path, I didn't know the purpose of each of them until Lafriks commented on them. I'll review what I did on that matter.

@lunny
Copy link
Member

lunny commented Nov 1, 2017

@dubeg Or could you give the permission to let maintainers to help you to push to this PR.

@dubeg
Copy link
Contributor Author

dubeg commented Nov 1, 2017

@lunny How can I do that?
Is it related to this checkbox?
editmaintainers

@lunny
Copy link
Member

lunny commented Nov 1, 2017

@dubeg yes

@dubeg
Copy link
Contributor Author

dubeg commented Nov 1, 2017

@lunny Well it's been checked for a while. If you want to finish to modify the PR, go ahead. Otherwise I'll work on it this weekend.

gdube and others added 2 commits November 3, 2017 09:09
The setting AppDataPath is now relative to the working directory.
The session svc's PROVIDER_CONFIG now defaults to AppDataPath/data/sessions.
The issue indexer's IssuePath now defaults to AppDataPath/indexers/issues.bleves.
@codecov-io
Copy link

codecov-io commented Nov 3, 2017

Codecov Report

Merging #2192 into master will increase coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2192      +/-   ##
==========================================
+ Coverage   26.84%   26.85%   +<.01%     
==========================================
  Files          89       89              
  Lines       17609    17607       -2     
==========================================
  Hits         4728     4728              
+ Misses      12195    12193       -2     
  Partials      686      686
Impacted Files Coverage Δ
models/models.go 28.31% <0%> (+0.24%) ⬆️

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 95637e0...fe26551. Read the comment docs.

@lunny
Copy link
Member

lunny commented Nov 3, 2017

@dubeg rebased and LGTM

@tboerger tboerger 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 Nov 3, 2017
@lafriks
Copy link
Member

lafriks commented Nov 3, 2017

LGTM

@tboerger tboerger 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 Nov 3, 2017
@lafriks lafriks merged commit 8798cf4 into go-gitea:master Nov 3, 2017
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
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. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants