From 7b3007bb48b436764dce49abade43b029442dbad Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Mon, 7 Aug 2023 22:02:47 +0200 Subject: [PATCH 01/11] add tests cases for attachments, lfs & actions_artifacts --- modules/setting/storage_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/modules/setting/storage_test.go b/modules/setting/storage_test.go index 9a83f31d918b4..8a20e82135079 100644 --- a/modules/setting/storage_test.go +++ b/modules/setting/storage_test.go @@ -117,6 +117,9 @@ func Test_getStorageInheritStorageTypeLocal(t *testing.T) { [storage] STORAGE_TYPE = local `, []testLocalStoragePathCase{ + {loadAttachmentFrom, &Attachment.Storage, "/appdata/attachments"}, + {loadLFSFrom, &LFS.Storage, "/appdata/lfs"}, + {loadActionsFrom, &Actions.ArtifactStorage, "/appdata/actions_artifacts"}, {loadPackagesFrom, &Packages.Storage, "/appdata/packages"}, {loadRepoArchiveFrom, &RepoArchive.Storage, "/appdata/repo-archive"}, {loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"}, @@ -131,6 +134,9 @@ func Test_getStorageInheritStorageTypeLocalPath(t *testing.T) { STORAGE_TYPE = local PATH = /data/gitea `, []testLocalStoragePathCase{ + {loadAttachmentFrom, &Attachment.Storage, "/data/gitea/attachments"}, + {loadLFSFrom, &LFS.Storage, "/data/gitea/lfs"}, + {loadActionsFrom, &Actions.ArtifactStorage, "/data/gitea/actions_artifacts"}, {loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"}, {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/repo-archive"}, {loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"}, @@ -145,6 +151,9 @@ func Test_getStorageInheritStorageTypeLocalRelativePath(t *testing.T) { STORAGE_TYPE = local PATH = storages `, []testLocalStoragePathCase{ + {loadAttachmentFrom, &Attachment.Storage, "/appdata/storages/attachments"}, + {loadLFSFrom, &LFS.Storage, "/appdata/storages/lfs"}, + {loadActionsFrom, &Actions.ArtifactStorage, "/appdata/storages/actions_artifacts"}, {loadPackagesFrom, &Packages.Storage, "/appdata/storages/packages"}, {loadRepoArchiveFrom, &RepoArchive.Storage, "/appdata/storages/repo-archive"}, {loadActionsFrom, &Actions.LogStorage, "/appdata/storages/actions_log"}, @@ -162,6 +171,9 @@ PATH = /data/gitea [repo-archive] PATH = /data/gitea/the-archives-dir `, []testLocalStoragePathCase{ + {loadAttachmentFrom, &Attachment.Storage, "/data/gitea/attachments"}, + {loadLFSFrom, &LFS.Storage, "/data/gitea/lfs"}, + {loadActionsFrom, &Actions.ArtifactStorage, "/data/gitea/actions_artifacts"}, {loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"}, {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/the-archives-dir"}, {loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"}, @@ -178,6 +190,9 @@ PATH = /data/gitea [repo-archive] `, []testLocalStoragePathCase{ + {loadAttachmentFrom, &Attachment.Storage, "/data/gitea/attachments"}, + {loadLFSFrom, &LFS.Storage, "/data/gitea/lfs"}, + {loadActionsFrom, &Actions.ArtifactStorage, "/data/gitea/actions_artifacts"}, {loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"}, {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/repo-archive"}, {loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"}, @@ -195,6 +210,9 @@ PATH = /data/gitea [repo-archive] PATH = the-archives-dir `, []testLocalStoragePathCase{ + {loadAttachmentFrom, &Attachment.Storage, "/data/gitea/attachments"}, + {loadLFSFrom, &LFS.Storage, "/data/gitea/lfs"}, + {loadActionsFrom, &Actions.ArtifactStorage, "/data/gitea/actions_artifacts"}, {loadPackagesFrom, &Packages.Storage, "/data/gitea/packages"}, {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/the-archives-dir"}, {loadActionsFrom, &Actions.LogStorage, "/data/gitea/actions_log"}, @@ -209,6 +227,9 @@ func Test_getStorageInheritStorageTypeLocalPathOverride3(t *testing.T) { STORAGE_TYPE = local PATH = /data/gitea/archives `, []testLocalStoragePathCase{ + {loadAttachmentFrom, &Attachment.Storage, "/appdata/attachments"}, + {loadLFSFrom, &LFS.Storage, "/appdata/lfs"}, + {loadActionsFrom, &Actions.ArtifactStorage, "/appdata/actions_artifacts"}, {loadPackagesFrom, &Packages.Storage, "/appdata/packages"}, {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/archives"}, {loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"}, @@ -226,6 +247,9 @@ PATH = /data/gitea/archives [repo-archive] PATH = /tmp/gitea/archives `, []testLocalStoragePathCase{ + {loadAttachmentFrom, &Attachment.Storage, "/appdata/attachments"}, + {loadLFSFrom, &LFS.Storage, "/appdata/lfs"}, + {loadActionsFrom, &Actions.ArtifactStorage, "/appdata/actions_artifacts"}, {loadPackagesFrom, &Packages.Storage, "/appdata/packages"}, {loadRepoArchiveFrom, &RepoArchive.Storage, "/tmp/gitea/archives"}, {loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"}, @@ -242,6 +266,9 @@ PATH = /data/gitea/archives [repo-archive] `, []testLocalStoragePathCase{ + {loadAttachmentFrom, &Attachment.Storage, "/appdata/attachments"}, + {loadLFSFrom, &LFS.Storage, "/appdata/lfs"}, + {loadActionsFrom, &Actions.ArtifactStorage, "/appdata/actions_artifacts"}, {loadPackagesFrom, &Packages.Storage, "/appdata/packages"}, {loadRepoArchiveFrom, &RepoArchive.Storage, "/data/gitea/archives"}, {loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"}, From 5dc55400c6ee9560e55aeb8831605b2a9afc27f0 Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Mon, 7 Aug 2023 22:10:47 +0200 Subject: [PATCH 02/11] test relative paths in [storage.xxx].PATH --- modules/setting/storage_test.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/modules/setting/storage_test.go b/modules/setting/storage_test.go index 8a20e82135079..bee133e99f4e7 100644 --- a/modules/setting/storage_test.go +++ b/modules/setting/storage_test.go @@ -238,6 +238,23 @@ PATH = /data/gitea/archives }) } +func Test_getStorageInheritStorageTypeLocalPathOverride3_5(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage.repo-archive] +STORAGE_TYPE = local +PATH = a-relative-path +`, []testLocalStoragePathCase{ + {loadAttachmentFrom, &Attachment.Storage, "/appdata/attachments"}, + {loadLFSFrom, &LFS.Storage, "/appdata/lfs"}, + {loadActionsFrom, &Actions.ArtifactStorage, "/appdata/actions_artifacts"}, + {loadPackagesFrom, &Packages.Storage, "/appdata/packages"}, + {loadRepoArchiveFrom, &RepoArchive.Storage, "/appdata/a-relative-path"}, + {loadActionsFrom, &Actions.LogStorage, "/appdata/actions_log"}, + {loadAvatarsFrom, &Avatar.Storage, "/appdata/avatars"}, + {loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/repo-avatars"}, + }) +} + func Test_getStorageInheritStorageTypeLocalPathOverride4(t *testing.T) { testLocalStoragePath(t, "/appdata", ` [storage.repo-archive] From 500c55aeb714e04077662b5bc0004a796f796791 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Thu, 10 Aug 2023 23:34:01 +0800 Subject: [PATCH 03/11] Fix storage derive configuration bug --- modules/setting/storage.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/setting/storage.go b/modules/setting/storage.go index c28f2be4ed714..edfeee1dcf82b 100644 --- a/modules/setting/storage.go +++ b/modules/setting/storage.go @@ -174,7 +174,7 @@ func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*S // extra config section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH, MINIO_BUCKET to override the targetsec when possible extraConfigSec := sec - if extraConfigSec == nil { + if extraConfigSec == nil && !targetSecIsStoragename { extraConfigSec = storageNameSec } From f485c7f78007712f0a073cbbbc245c573078e2a8 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 11 Aug 2023 00:02:36 +0800 Subject: [PATCH 04/11] Fix bug --- modules/setting/packages_test.go | 1 + modules/setting/storage.go | 1 + 2 files changed, 2 insertions(+) diff --git a/modules/setting/packages_test.go b/modules/setting/packages_test.go index 87de276041764..13d3efe4f7b1c 100644 --- a/modules/setting/packages_test.go +++ b/modules/setting/packages_test.go @@ -176,6 +176,7 @@ func Test_PackageStorage4(t *testing.T) { STORAGE_TYPE = my_cfg MINIO_BASE_PATH = my_packages/ SERVE_DIRECT = true + [storage.my_cfg] STORAGE_TYPE = minio MINIO_ENDPOINT = s3.my-domain.net diff --git a/modules/setting/storage.go b/modules/setting/storage.go index edfeee1dcf82b..b6d5a6f1474d5 100644 --- a/modules/setting/storage.go +++ b/modules/setting/storage.go @@ -150,6 +150,7 @@ func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*S } } default: + targetSecIsStoragename = false newTargetSec, _ := rootCfg.GetSection(storageSectionName + "." + targetType) if newTargetSec == nil { if !IsValidStorageType(StorageType(targetType)) { From 1f34c94cfb01d0dfa70648f59edf63c13c99da4e Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 11 Aug 2023 12:24:32 +0800 Subject: [PATCH 05/11] Rewrite the getStorage function --- modules/setting/storage.go | 239 ++++++++++++++++++-------------- modules/setting/storage_test.go | 10 ++ 2 files changed, 146 insertions(+), 103 deletions(-) diff --git a/modules/setting/storage.go b/modules/setting/storage.go index b6d5a6f1474d5..2f7781c3cff56 100644 --- a/modules/setting/storage.go +++ b/modules/setting/storage.go @@ -84,142 +84,175 @@ func getDefaultStorageSection(rootCfg ConfigProvider) ConfigSection { return storageSec } -// getStorage will find target section and extra special section first and then read override -// items from extra section -func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*Storage, error) { - if name == "" { - return nil, errors.New("no name for storage") +func getStorageByType(rootCfg ConfigProvider, typ string) (ConfigSection, int, error) { + targetSec, err := rootCfg.GetSection(storageSectionName + "." + typ) + if err != nil { + if !IsValidStorageType(StorageType(typ)) { + return nil, 0, fmt.Errorf("get section via storage type %q failed: %v", typ, err) + } + } + if targetSec == nil { + return nil, 0, nil } - var targetSec ConfigSection - // check typ first - if typ != "" { - var err error - targetSec, err = rootCfg.GetSection(storageSectionName + "." + typ) - if err != nil { - if !IsValidStorageType(StorageType(typ)) { - return nil, fmt.Errorf("get section via storage type %q failed: %v", typ, err) - } + targetType := targetSec.Key("STORAGE_TYPE").String() + if targetType == "" { + if !IsValidStorageType(StorageType(typ)) { + return nil, 0, fmt.Errorf("unknow storage type %q", typ) } - if targetSec != nil { - targetType := targetSec.Key("STORAGE_TYPE").String() - if targetType == "" { - if !IsValidStorageType(StorageType(typ)) { - return nil, fmt.Errorf("unknow storage type %q", typ) + targetSec.Key("STORAGE_TYPE").SetValue(typ) + } else if !IsValidStorageType(StorageType(targetType)) { + return nil, 0, fmt.Errorf("unknow storage type %q for section storage.%v", targetType, typ) + } + + return targetSec, targetSecIsTyp, nil +} + +func getStorageTargetSec(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (ConfigSection, int, error) { + // check typ first + if typ == "" { + if sec != nil { // check sec's type secondly + typ = sec.Key("STORAGE_TYPE").String() + if IsValidStorageType(StorageType(typ)) { + if targetSec, _ := rootCfg.GetSection(storageSectionName + "." + typ); targetSec == nil { + return sec, targetSecIsSec, nil } - targetSec.Key("STORAGE_TYPE").SetValue(typ) - } else if !IsValidStorageType(StorageType(targetType)) { - return nil, fmt.Errorf("unknow storage type %q for section storage.%v", targetType, typ) } } } - if targetSec == nil && sec != nil { - secTyp := sec.Key("STORAGE_TYPE").String() - if IsValidStorageType(StorageType(secTyp)) { - targetSec = sec - } else if secTyp != "" { - targetSec, _ = rootCfg.GetSection(storageSectionName + "." + secTyp) + if typ != "" { + targetSec, tp, err := getStorageByType(rootCfg, typ) + if targetSec != nil || err != nil { + return targetSec, tp, err } } - targetSecIsStoragename := false - storageNameSec, _ := rootCfg.GetSection(storageSectionName + "." + name) - if targetSec == nil { - targetSec = storageNameSec - targetSecIsStoragename = storageNameSec != nil - } - - if targetSec == nil { - targetSec = getDefaultStorageSection(rootCfg) - } else { + // check stoarge name thirdly + targetSec, _ := rootCfg.GetSection(storageSectionName + "." + name) + if targetSec != nil { targetType := targetSec.Key("STORAGE_TYPE").String() switch { case targetType == "": - if targetSec != storageNameSec && storageNameSec != nil { - targetSec = storageNameSec - targetSecIsStoragename = true - if targetSec.Key("STORAGE_TYPE").String() == "" { - return nil, fmt.Errorf("storage section %s.%s has no STORAGE_TYPE", storageSectionName, name) - } - } else { - if targetSec.Key("PATH").String() == "" { - targetSec = getDefaultStorageSection(rootCfg) - } else { - targetSec.Key("STORAGE_TYPE").SetValue("local") - } + if targetSec.Key("PATH").String() == "" { // both storage type and path are empty, use default + return getDefaultStorageSection(rootCfg), targetSecIsDefault, nil } + + targetSec.Key("STORAGE_TYPE").SetValue("local") default: - targetSecIsStoragename = false - newTargetSec, _ := rootCfg.GetSection(storageSectionName + "." + targetType) - if newTargetSec == nil { - if !IsValidStorageType(StorageType(targetType)) { - return nil, fmt.Errorf("invalid storage section %s.%q", storageSectionName, targetType) - } - } else { - targetSec = newTargetSec - if IsValidStorageType(StorageType(targetType)) { - tp := targetSec.Key("STORAGE_TYPE").String() - if tp == "" { - targetSec.Key("STORAGE_TYPE").SetValue(targetType) - } - } + targetSec, tp, err := getStorageByType(rootCfg, targetType) + if targetSec != nil || err != nil { + return targetSec, tp, err } } + + return targetSec, targetSecIsStorageWithName, nil } - targetType := targetSec.Key("STORAGE_TYPE").String() - if !IsValidStorageType(StorageType(targetType)) { - return nil, fmt.Errorf("invalid storage type %q", targetType) + return getDefaultStorageSection(rootCfg), targetSecIsDefault, nil +} + +const ( + targetSecIsTyp = iota + targetSecIsStorage + targetSecIsDefault + targetSecIsStorageWithName + targetSecIsSec +) + +// getOverrideSection override section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH, MINIO_BUCKET to override the targetsec when possible +func getOverrideSection(rootConfig ConfigProvider, targetSec, sec ConfigSection, targetSecType int, name string) (ConfigSection, error) { + if targetSecType == targetSecIsSec { + return nil, nil } - // extra config section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH, MINIO_BUCKET to override the targetsec when possible - extraConfigSec := sec - if extraConfigSec == nil && !targetSecIsStoragename { - extraConfigSec = storageNameSec + if sec != nil { + return sec, nil } + if targetSecType != targetSecIsStorageWithName { + nameSec, _ := rootConfig.GetSection(storageSectionName + "." + name) + return nameSec, nil + } + return nil, nil +} + +func getStorageForLocal(targetSec, overrideSec ConfigSection, tp int, name string) (*Storage, error) { var storage Storage - storage.Type = StorageType(targetType) + storage.Type = StorageType(targetSec.Key("STORAGE_TYPE").String()) - switch targetType { - case string(LocalStorageType): - targetPath := ConfigSectionKeyString(targetSec, "PATH", "") - if targetPath == "" { - targetPath = AppDataPath - } else if !filepath.IsAbs(targetPath) { - targetPath = filepath.Join(AppDataPath, targetPath) - } + targetPath := ConfigSectionKeyString(targetSec, "PATH", "") + if targetPath == "" { + targetPath = AppDataPath + } else if !filepath.IsAbs(targetPath) { + targetPath = filepath.Join(AppDataPath, targetPath) + } - var fallbackPath string - if targetSecIsStoragename { - fallbackPath = targetPath - } else { - fallbackPath = filepath.Join(targetPath, name) - } + var fallbackPath string + if tp == targetSecIsStorage || tp == targetSecIsDefault { + fallbackPath = filepath.Join(targetPath, name) + } else { + fallbackPath = targetPath + } - if extraConfigSec == nil { - storage.Path = fallbackPath - } else { - storage.Path = ConfigSectionKeyString(extraConfigSec, "PATH", fallbackPath) - if !filepath.IsAbs(storage.Path) { - storage.Path = filepath.Join(targetPath, storage.Path) - } + if overrideSec == nil { + storage.Path = fallbackPath + } else { + storage.Path = ConfigSectionKeyString(overrideSec, "PATH", fallbackPath) + if !filepath.IsAbs(storage.Path) { + storage.Path = filepath.Join(targetPath, storage.Path) } + } + return &storage, nil +} - case string(MinioStorageType): - if err := targetSec.MapTo(&storage.MinioConfig); err != nil { - return nil, fmt.Errorf("map minio config failed: %v", err) - } +func getStorageForMinio(targetSec, overrideSec ConfigSection, tp int, name string) (*Storage, error) { + var storage Storage + storage.Type = StorageType(targetSec.Key("STORAGE_TYPE").String()) + if err := targetSec.MapTo(&storage.MinioConfig); err != nil { + return nil, fmt.Errorf("map minio config failed: %v", err) + } + if storage.MinioConfig.BasePath == "" { storage.MinioConfig.BasePath = name + "/" - - if extraConfigSec != nil { - storage.MinioConfig.ServeDirect = ConfigSectionKeyBool(extraConfigSec, "SERVE_DIRECT", storage.MinioConfig.ServeDirect) - storage.MinioConfig.BasePath = ConfigSectionKeyString(extraConfigSec, "MINIO_BASE_PATH", storage.MinioConfig.BasePath) - storage.MinioConfig.Bucket = ConfigSectionKeyString(extraConfigSec, "MINIO_BUCKET", storage.MinioConfig.Bucket) - } } + if overrideSec != nil { + storage.MinioConfig.ServeDirect = ConfigSectionKeyBool(overrideSec, "SERVE_DIRECT", storage.MinioConfig.ServeDirect) + storage.MinioConfig.BasePath = ConfigSectionKeyString(overrideSec, "MINIO_BASE_PATH", storage.MinioConfig.BasePath) + storage.MinioConfig.Bucket = ConfigSectionKeyString(overrideSec, "MINIO_BUCKET", storage.MinioConfig.Bucket) + } return &storage, nil } + +// getStorage will find target section and extra special section first and then read override +// items from extra section +func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*Storage, error) { + if name == "" { + return nil, errors.New("no name for storage") + } + + targetSec, tp, err := getStorageTargetSec(rootCfg, name, typ, sec) + if err != nil { + return nil, err + } + + targetType := targetSec.Key("STORAGE_TYPE").String() + if !IsValidStorageType(StorageType(targetType)) { + return nil, fmt.Errorf("invalid storage type %q", targetType) + } + + overrideSec, err := getOverrideSection(rootCfg, targetSec, sec, tp, name) + if err != nil { + return nil, err + } + + switch targetType { + case string(LocalStorageType): + return getStorageForLocal(targetSec, overrideSec, tp, name) + case string(MinioStorageType): + return getStorageForMinio(targetSec, overrideSec, tp, name) + default: + return nil, fmt.Errorf("unsupported storage type %q", targetType) + } +} diff --git a/modules/setting/storage_test.go b/modules/setting/storage_test.go index bee133e99f4e7..15161dd64e8e4 100644 --- a/modules/setting/storage_test.go +++ b/modules/setting/storage_test.go @@ -293,3 +293,13 @@ PATH = /data/gitea/archives {loadRepoAvatarFrom, &RepoAvatar.Storage, "/appdata/repo-avatars"}, }) } + +func Test_getStorageInheritStorageTypeLocalPathOverride72(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[repo-archive] +STORAGE_TYPE = local +PATH = archives +`, []testLocalStoragePathCase{ + {loadRepoArchiveFrom, &RepoArchive.Storage, "/appdata/archives"}, + }) +} From 779aaf4c13f6039b817e576935356b8ad923958b Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 11 Aug 2023 12:30:40 +0800 Subject: [PATCH 06/11] Move functions to make it easier to review --- modules/setting/packages_test.go | 1 - modules/setting/storage.go | 80 ++++++++++++++++---------------- 2 files changed, 40 insertions(+), 41 deletions(-) diff --git a/modules/setting/packages_test.go b/modules/setting/packages_test.go index 13d3efe4f7b1c..87de276041764 100644 --- a/modules/setting/packages_test.go +++ b/modules/setting/packages_test.go @@ -176,7 +176,6 @@ func Test_PackageStorage4(t *testing.T) { STORAGE_TYPE = my_cfg MINIO_BASE_PATH = my_packages/ SERVE_DIRECT = true - [storage.my_cfg] STORAGE_TYPE = minio MINIO_ENDPOINT = s3.my-domain.net diff --git a/modules/setting/storage.go b/modules/setting/storage.go index 2f7781c3cff56..21d0a30b2cd6a 100644 --- a/modules/setting/storage.go +++ b/modules/setting/storage.go @@ -84,6 +84,46 @@ func getDefaultStorageSection(rootCfg ConfigProvider) ConfigSection { return storageSec } +// getStorage will find target section and extra special section first and then read override +// items from extra section +func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*Storage, error) { + if name == "" { + return nil, errors.New("no name for storage") + } + + targetSec, tp, err := getStorageTargetSec(rootCfg, name, typ, sec) + if err != nil { + return nil, err + } + + targetType := targetSec.Key("STORAGE_TYPE").String() + if !IsValidStorageType(StorageType(targetType)) { + return nil, fmt.Errorf("invalid storage type %q", targetType) + } + + overrideSec, err := getOverrideSection(rootCfg, targetSec, sec, tp, name) + if err != nil { + return nil, err + } + + switch targetType { + case string(LocalStorageType): + return getStorageForLocal(targetSec, overrideSec, tp, name) + case string(MinioStorageType): + return getStorageForMinio(targetSec, overrideSec, tp, name) + default: + return nil, fmt.Errorf("unsupported storage type %q", targetType) + } +} + +const ( + targetSecIsTyp = iota // target section is [storage.type] which the type from parameter + targetSecIsStorage // target section is [storage] + targetSecIsDefault // target section is the default value + targetSecIsStorageWithName // target section is [storage.name] + targetSecIsSec // target section is from the name seciont [name] +) + func getStorageByType(rootCfg ConfigProvider, typ string) (ConfigSection, int, error) { targetSec, err := rootCfg.GetSection(storageSectionName + "." + typ) if err != nil { @@ -152,14 +192,6 @@ func getStorageTargetSec(rootCfg ConfigProvider, name, typ string, sec ConfigSec return getDefaultStorageSection(rootCfg), targetSecIsDefault, nil } -const ( - targetSecIsTyp = iota - targetSecIsStorage - targetSecIsDefault - targetSecIsStorageWithName - targetSecIsSec -) - // getOverrideSection override section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH, MINIO_BUCKET to override the targetsec when possible func getOverrideSection(rootConfig ConfigProvider, targetSec, sec ConfigSection, targetSecType int, name string) (ConfigSection, error) { if targetSecType == targetSecIsSec { @@ -224,35 +256,3 @@ func getStorageForMinio(targetSec, overrideSec ConfigSection, tp int, name strin } return &storage, nil } - -// getStorage will find target section and extra special section first and then read override -// items from extra section -func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*Storage, error) { - if name == "" { - return nil, errors.New("no name for storage") - } - - targetSec, tp, err := getStorageTargetSec(rootCfg, name, typ, sec) - if err != nil { - return nil, err - } - - targetType := targetSec.Key("STORAGE_TYPE").String() - if !IsValidStorageType(StorageType(targetType)) { - return nil, fmt.Errorf("invalid storage type %q", targetType) - } - - overrideSec, err := getOverrideSection(rootCfg, targetSec, sec, tp, name) - if err != nil { - return nil, err - } - - switch targetType { - case string(LocalStorageType): - return getStorageForLocal(targetSec, overrideSec, tp, name) - case string(MinioStorageType): - return getStorageForMinio(targetSec, overrideSec, tp, name) - default: - return nil, fmt.Errorf("unsupported storage type %q", targetType) - } -} From b7c10f4c5119282e15e100a3c0c488fbf7e443b6 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 11 Aug 2023 16:15:29 +0800 Subject: [PATCH 07/11] More tests for getStorage related functions --- modules/setting/storage.go | 33 +++++-------- modules/setting/storage_test.go | 88 +++++++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+), 20 deletions(-) diff --git a/modules/setting/storage.go b/modules/setting/storage.go index 21d0a30b2cd6a..a11790363d8b4 100644 --- a/modules/setting/storage.go +++ b/modules/setting/storage.go @@ -91,21 +91,14 @@ func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*S return nil, errors.New("no name for storage") } - targetSec, tp, err := getStorageTargetSec(rootCfg, name, typ, sec) + targetSec, tp, err := getStorageTargetSection(rootCfg, name, typ, sec) if err != nil { return nil, err } - targetType := targetSec.Key("STORAGE_TYPE").String() - if !IsValidStorageType(StorageType(targetType)) { - return nil, fmt.Errorf("invalid storage type %q", targetType) - } - - overrideSec, err := getOverrideSection(rootCfg, targetSec, sec, tp, name) - if err != nil { - return nil, err - } + overrideSec := getStorageOverrideSection(rootCfg, targetSec, sec, tp, name) + targetType := targetSec.Key("STORAGE_TYPE").String() switch targetType { case string(LocalStorageType): return getStorageForLocal(targetSec, overrideSec, tp, name) @@ -124,7 +117,7 @@ const ( targetSecIsSec // target section is from the name seciont [name] ) -func getStorageByType(rootCfg ConfigProvider, typ string) (ConfigSection, int, error) { +func getStorageSectionByType(rootCfg ConfigProvider, typ string) (ConfigSection, int, error) { targetSec, err := rootCfg.GetSection(storageSectionName + "." + typ) if err != nil { if !IsValidStorageType(StorageType(typ)) { @@ -148,7 +141,7 @@ func getStorageByType(rootCfg ConfigProvider, typ string) (ConfigSection, int, e return targetSec, targetSecIsTyp, nil } -func getStorageTargetSec(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (ConfigSection, int, error) { +func getStorageTargetSection(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (ConfigSection, int, error) { // check typ first if typ == "" { if sec != nil { // check sec's type secondly @@ -162,7 +155,7 @@ func getStorageTargetSec(rootCfg ConfigProvider, name, typ string, sec ConfigSec } if typ != "" { - targetSec, tp, err := getStorageByType(rootCfg, typ) + targetSec, tp, err := getStorageSectionByType(rootCfg, typ) if targetSec != nil || err != nil { return targetSec, tp, err } @@ -180,7 +173,7 @@ func getStorageTargetSec(rootCfg ConfigProvider, name, typ string, sec ConfigSec targetSec.Key("STORAGE_TYPE").SetValue("local") default: - targetSec, tp, err := getStorageByType(rootCfg, targetType) + targetSec, tp, err := getStorageSectionByType(rootCfg, targetType) if targetSec != nil || err != nil { return targetSec, tp, err } @@ -192,21 +185,21 @@ func getStorageTargetSec(rootCfg ConfigProvider, name, typ string, sec ConfigSec return getDefaultStorageSection(rootCfg), targetSecIsDefault, nil } -// getOverrideSection override section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH, MINIO_BUCKET to override the targetsec when possible -func getOverrideSection(rootConfig ConfigProvider, targetSec, sec ConfigSection, targetSecType int, name string) (ConfigSection, error) { +// getStorageOverrideSection override section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH, MINIO_BUCKET to override the targetsec when possible +func getStorageOverrideSection(rootConfig ConfigProvider, targetSec, sec ConfigSection, targetSecType int, name string) ConfigSection { if targetSecType == targetSecIsSec { - return nil, nil + return nil } if sec != nil { - return sec, nil + return sec } if targetSecType != targetSecIsStorageWithName { nameSec, _ := rootConfig.GetSection(storageSectionName + "." + name) - return nameSec, nil + return nameSec } - return nil, nil + return nil } func getStorageForLocal(targetSec, overrideSec ConfigSection, tp int, name string) (*Storage, error) { diff --git a/modules/setting/storage_test.go b/modules/setting/storage_test.go index 15161dd64e8e4..ff0f0f07f6c6d 100644 --- a/modules/setting/storage_test.go +++ b/modules/setting/storage_test.go @@ -303,3 +303,91 @@ PATH = archives {loadRepoArchiveFrom, &RepoArchive.Storage, "/appdata/archives"}, }) } + +func Test_getStorageConfiguration20(t *testing.T) { + cfg, err := NewConfigProviderFromData(` +[repo-archive] +STORAGE_TYPE = my_storage +PATH = archives +`) + assert.NoError(t, err) + + assert.Error(t, loadRepoArchiveFrom(cfg)) +} + +func Test_getStorageConfiguration21(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage.repo-archive] +`, []testLocalStoragePathCase{ + {loadRepoArchiveFrom, &RepoArchive.Storage, "/appdata/repo-archive"}, + }) +} + +func Test_getStorageConfiguration22(t *testing.T) { + testLocalStoragePath(t, "/appdata", ` +[storage.repo-archive] +PATH = archives +`, []testLocalStoragePathCase{ + {loadRepoArchiveFrom, &RepoArchive.Storage, "/appdata/archives"}, + }) +} + +func Test_getStorageConfiguration23(t *testing.T) { + cfg, err := NewConfigProviderFromData(` +[repo-archive] +STORAGE_TYPE = minio +MINIO_ACCESS_KEY_ID = my_access_key +MINIO_SECRET_ACCESS_KEY = my_secret_key +`) + assert.NoError(t, err) + + _, err = getStorage(cfg, "", "", nil) + assert.Error(t, err) + + assert.NoError(t, loadRepoArchiveFrom(cfg)) + cp := RepoArchive.Storage.ToShadowCopy() + assert.EqualValues(t, "******", cp.MinioConfig.AccessKeyID) + assert.EqualValues(t, "******", cp.MinioConfig.SecretAccessKey) +} + +func Test_getStorageConfiguration24(t *testing.T) { + cfg, err := NewConfigProviderFromData(` +[repo-archive] +STORAGE_TYPE = my_archive + +[storage.my_archive] +; unsupported, storage type should be defined explicitly +PATH = archives +`) + assert.NoError(t, err) + assert.Error(t, loadRepoArchiveFrom(cfg)) +} + +func Test_getStorageConfiguration25(t *testing.T) { + cfg, err := NewConfigProviderFromData(` +[repo-archive] +STORAGE_TYPE = my_archive + +[storage.my_archive] +; unsupported, storage type should be known type +STORAGE_TYPE = unknown // should be local or minio +PATH = archives +`) + assert.NoError(t, err) + assert.Error(t, loadRepoArchiveFrom(cfg)) +} + +func Test_getStorageConfiguration26(t *testing.T) { + cfg, err := NewConfigProviderFromData(` +[repo-archive] +STORAGE_TYPE = minio +MINIO_ACCESS_KEY_ID = my_access_key +MINIO_SECRET_ACCESS_KEY = my_secret_key +; wrong configuration +MINIO_USE_SSL = abc +`) + assert.NoError(t, err) + // assert.Error(t, loadRepoArchiveFrom(cfg)) + // FIXME: this should return error but now ini package's MapTo() doesn't check type + assert.NoError(t, loadRepoArchiveFrom(cfg)) +} From 658ee02773e5e30bdb6d8a90594bcb18b647847a Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Fri, 11 Aug 2023 16:30:51 +0800 Subject: [PATCH 08/11] Add more test --- modules/setting/storage_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/modules/setting/storage_test.go b/modules/setting/storage_test.go index ff0f0f07f6c6d..20886d4c4e959 100644 --- a/modules/setting/storage_test.go +++ b/modules/setting/storage_test.go @@ -27,12 +27,15 @@ MINIO_BUCKET = gitea-storage assert.NoError(t, loadAttachmentFrom(cfg)) assert.EqualValues(t, "gitea-attachment", Attachment.Storage.MinioConfig.Bucket) + assert.EqualValues(t, "attachments/", Attachment.Storage.MinioConfig.BasePath) assert.NoError(t, loadLFSFrom(cfg)) assert.EqualValues(t, "gitea-lfs", LFS.Storage.MinioConfig.Bucket) + assert.EqualValues(t, "lfs/", LFS.Storage.MinioConfig.BasePath) assert.NoError(t, loadAvatarsFrom(cfg)) assert.EqualValues(t, "gitea-storage", Avatar.Storage.MinioConfig.Bucket) + assert.EqualValues(t, "avatars/", Avatar.Storage.MinioConfig.BasePath) } func Test_getStorageUseOtherNameAsType(t *testing.T) { @@ -49,9 +52,11 @@ MINIO_BUCKET = gitea-storage assert.NoError(t, loadAttachmentFrom(cfg)) assert.EqualValues(t, "gitea-storage", Attachment.Storage.MinioConfig.Bucket) + assert.EqualValues(t, "attachments/", Attachment.Storage.MinioConfig.BasePath) assert.NoError(t, loadLFSFrom(cfg)) assert.EqualValues(t, "gitea-storage", LFS.Storage.MinioConfig.Bucket) + assert.EqualValues(t, "lfs/", LFS.Storage.MinioConfig.BasePath) } func Test_getStorageInheritStorageType(t *testing.T) { @@ -391,3 +396,19 @@ MINIO_USE_SSL = abc // FIXME: this should return error but now ini package's MapTo() doesn't check type assert.NoError(t, loadRepoArchiveFrom(cfg)) } + +func Test_getStorageConfiguration27(t *testing.T) { + cfg, err := NewConfigProviderFromData(` +[storage.repo-archive] +STORAGE_TYPE = minio +MINIO_ACCESS_KEY_ID = my_access_key +MINIO_SECRET_ACCESS_KEY = my_secret_key +MINIO_USE_SSL = true +`) + assert.NoError(t, err) + assert.NoError(t, loadRepoArchiveFrom(cfg)) + assert.EqualValues(t, "my_access_key", RepoArchive.Storage.MinioConfig.AccessKeyID) + assert.EqualValues(t, "my_secret_key", RepoArchive.Storage.MinioConfig.SecretAccessKey) + assert.EqualValues(t, true, RepoArchive.Storage.MinioConfig.UseSSL) + assert.EqualValues(t, "repo-archive/", RepoArchive.Storage.MinioConfig.BasePath) +} From db1d26af43f93c16ef8e356971ebd3d29399982c Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 12 Aug 2023 20:59:06 +0800 Subject: [PATCH 09/11] use a type --- modules/setting/storage.go | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/modules/setting/storage.go b/modules/setting/storage.go index a11790363d8b4..e11c1592845df 100644 --- a/modules/setting/storage.go +++ b/modules/setting/storage.go @@ -109,22 +109,24 @@ func getStorage(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (*S } } +type targetSecType int + const ( - targetSecIsTyp = iota // target section is [storage.type] which the type from parameter - targetSecIsStorage // target section is [storage] - targetSecIsDefault // target section is the default value - targetSecIsStorageWithName // target section is [storage.name] - targetSecIsSec // target section is from the name seciont [name] + targetSecIsTyp targetSecType = iota // target section is [storage.type] which the type from parameter + targetSecIsStorage // target section is [storage] + targetSecIsDefault // target section is the default value + targetSecIsStorageWithName // target section is [storage.name] + targetSecIsSec // target section is from the name seciont [name] ) -func getStorageSectionByType(rootCfg ConfigProvider, typ string) (ConfigSection, int, error) { +func getStorageSectionByType(rootCfg ConfigProvider, typ string) (ConfigSection, targetSecType, error) { targetSec, err := rootCfg.GetSection(storageSectionName + "." + typ) if err != nil { if !IsValidStorageType(StorageType(typ)) { return nil, 0, fmt.Errorf("get section via storage type %q failed: %v", typ, err) } - } - if targetSec == nil { + // if typ is a valid storage type, but there is no [storage.local] or [storage.minio] section + // it's not an error return nil, 0, nil } @@ -141,7 +143,7 @@ func getStorageSectionByType(rootCfg ConfigProvider, typ string) (ConfigSection, return targetSec, targetSecIsTyp, nil } -func getStorageTargetSection(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (ConfigSection, int, error) { +func getStorageTargetSection(rootCfg ConfigProvider, name, typ string, sec ConfigSection) (ConfigSection, targetSecType, error) { // check typ first if typ == "" { if sec != nil { // check sec's type secondly @@ -186,7 +188,7 @@ func getStorageTargetSection(rootCfg ConfigProvider, name, typ string, sec Confi } // getStorageOverrideSection override section will be read SERVE_DIRECT, PATH, MINIO_BASE_PATH, MINIO_BUCKET to override the targetsec when possible -func getStorageOverrideSection(rootConfig ConfigProvider, targetSec, sec ConfigSection, targetSecType int, name string) ConfigSection { +func getStorageOverrideSection(rootConfig ConfigProvider, targetSec, sec ConfigSection, targetSecType targetSecType, name string) ConfigSection { if targetSecType == targetSecIsSec { return nil } @@ -202,7 +204,7 @@ func getStorageOverrideSection(rootConfig ConfigProvider, targetSec, sec ConfigS return nil } -func getStorageForLocal(targetSec, overrideSec ConfigSection, tp int, name string) (*Storage, error) { +func getStorageForLocal(targetSec, overrideSec ConfigSection, tp targetSecType, name string) (*Storage, error) { var storage Storage storage.Type = StorageType(targetSec.Key("STORAGE_TYPE").String()) @@ -231,7 +233,7 @@ func getStorageForLocal(targetSec, overrideSec ConfigSection, tp int, name strin return &storage, nil } -func getStorageForMinio(targetSec, overrideSec ConfigSection, tp int, name string) (*Storage, error) { +func getStorageForMinio(targetSec, overrideSec ConfigSection, tp targetSecType, name string) (*Storage, error) { var storage Storage storage.Type = StorageType(targetSec.Key("STORAGE_TYPE").String()) if err := targetSec.MapTo(&storage.MinioConfig); err != nil { From 31f9a55dbcf35919a1a62e701d989bf071b5b471 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 12 Aug 2023 21:06:51 +0800 Subject: [PATCH 10/11] refactor getStorageForLocal --- modules/setting/storage.go | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/modules/setting/storage.go b/modules/setting/storage.go index e11c1592845df..a2f827c8773de 100644 --- a/modules/setting/storage.go +++ b/modules/setting/storage.go @@ -205,30 +205,29 @@ func getStorageOverrideSection(rootConfig ConfigProvider, targetSec, sec ConfigS } func getStorageForLocal(targetSec, overrideSec ConfigSection, tp targetSecType, name string) (*Storage, error) { - var storage Storage - storage.Type = StorageType(targetSec.Key("STORAGE_TYPE").String()) - - targetPath := ConfigSectionKeyString(targetSec, "PATH", "") - if targetPath == "" { - targetPath = AppDataPath - } else if !filepath.IsAbs(targetPath) { - targetPath = filepath.Join(AppDataPath, targetPath) + storage := Storage{ + Type: StorageType(targetSec.Key("STORAGE_TYPE").String()), } + targetPath := ConfigSectionKeyString(targetSec, "PATH", "") var fallbackPath string - if tp == targetSecIsStorage || tp == targetSecIsDefault { - fallbackPath = filepath.Join(targetPath, name) + if targetPath == "" { // no path + fallbackPath = filepath.Join(AppDataPath, name) } else { - fallbackPath = targetPath + if tp == targetSecIsStorage || tp == targetSecIsDefault { + fallbackPath = filepath.Join(targetPath, name) + } else { + fallbackPath = targetPath + } } if overrideSec == nil { storage.Path = fallbackPath } else { storage.Path = ConfigSectionKeyString(overrideSec, "PATH", fallbackPath) - if !filepath.IsAbs(storage.Path) { - storage.Path = filepath.Join(targetPath, storage.Path) - } + } + if !filepath.IsAbs(storage.Path) { + storage.Path = filepath.Join(AppDataPath, storage.Path) } return &storage, nil } From 982f241437d76befba876b6be5ef317507d5c139 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 12 Aug 2023 22:44:29 +0800 Subject: [PATCH 11/11] Fix test --- modules/setting/storage.go | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/modules/setting/storage.go b/modules/setting/storage.go index a2f827c8773de..cc3a2899d76ed 100644 --- a/modules/setting/storage.go +++ b/modules/setting/storage.go @@ -219,16 +219,26 @@ func getStorageForLocal(targetSec, overrideSec ConfigSection, tp targetSecType, } else { fallbackPath = targetPath } + if !filepath.IsAbs(fallbackPath) { + fallbackPath = filepath.Join(AppDataPath, fallbackPath) + } } - if overrideSec == nil { + if overrideSec == nil { // no override section storage.Path = fallbackPath } else { - storage.Path = ConfigSectionKeyString(overrideSec, "PATH", fallbackPath) - } - if !filepath.IsAbs(storage.Path) { - storage.Path = filepath.Join(AppDataPath, storage.Path) + storage.Path = ConfigSectionKeyString(overrideSec, "PATH", "") + if storage.Path == "" { // overrideSec has no path + storage.Path = fallbackPath + } else if !filepath.IsAbs(storage.Path) { + if targetPath == "" { + storage.Path = filepath.Join(AppDataPath, storage.Path) + } else { + storage.Path = filepath.Join(targetPath, storage.Path) + } + } } + return &storage, nil }