Skip to content

Conversation

@lunny
Copy link
Member

@lunny lunny commented Sep 11, 2020

  • Add default storage configurations
  • Moved LFS storage configurations from LFS_* to under [lfs]
  • Also includes some bugs of LFS path
  • Fix bugs on dump related to Attachment and LFS
  • Some break changes related to Attachment and LFS on recent PRs

@6543 6543 added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Sep 11, 2020
@lunny lunny force-pushed the lunny/storage_config branch from 422b2be to f8905a5 Compare September 11, 2020 13:16
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 11, 2020
@lunny lunny changed the title Add default storage configurations WIP: Add default storage configurations Sep 11, 2020
@lunny lunny added the type/bug label Sep 11, 2020
@lunny lunny added this to the 1.13.0 milestone Sep 11, 2020
@lunny lunny force-pushed the lunny/storage_config branch 2 times, most recently from 0cf27dc to 469a28b Compare September 13, 2020 13:33
@lunny lunny changed the title WIP: Add default storage configurations Add default storage configurations Sep 13, 2020
@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2020

Codecov Report

Merging #12813 into master will increase coverage by 0.03%.
The diff coverage is 30.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12813      +/-   ##
==========================================
+ Coverage   42.54%   42.58%   +0.03%     
==========================================
  Files         670      671       +1     
  Lines       73522    73627     +105     
==========================================
+ Hits        31281    31354      +73     
- Misses      37175    37188      +13     
- Partials     5066     5085      +19     
Impacted Files Coverage Δ
cmd/dump.go 0.90% <0.00%> (-0.13%) ⬇️
cmd/migrate_storage.go 0.00% <0.00%> (ø)
models/admin.go 69.09% <0.00%> (-2.61%) ⬇️
models/repo.go 55.15% <0.00%> (+0.04%) ⬆️
modules/storage/minio.go 0.00% <0.00%> (-48.44%) ⬇️
routers/install.go 0.00% <0.00%> (ø)
routers/repo/lfs.go 0.00% <0.00%> (ø)
modules/storage/local.go 38.09% <5.88%> (-11.91%) ⬇️
modules/setting/storage.go 43.47% <43.47%> (ø)
modules/setting/attachment.go 64.28% <55.00%> (-16.67%) ⬇️
... and 28 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 4c6ac08...dcd73d1. Read the comment docs.

@lafriks
Copy link
Member

lafriks commented Sep 14, 2020

We should probably prefix filenames with type (ex. lfs-, avatar-, repo-avatar- etc) so that all blobs can be stored in single bucket

@lunny
Copy link
Member Author

lunny commented Sep 15, 2020

This also fixes #12828

@lunny
Copy link
Member Author

lunny commented Sep 15, 2020

@lafriks MINIO_BASE_PATH will resolve the problem.

@GiteaBot GiteaBot 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 Sep 16, 2020
.golangci.yml Outdated
Comment on lines 101 to 103
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this?

This is a bad sign, we should be removing lint exceptions not adding them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed

- path: modules/setting/attachment.go
      linters:
        - dupl
    - path: modules/setting/lfs.go
      linters:
        - dupl

but still keep cmd/dump.go because it reports the duplicated codes but I think merging them as a function is not a good choice.

@lunny lunny force-pushed the lunny/storage_config branch from bd58ab2 to 3c9f56f Compare September 18, 2020 02:37
@lunny
Copy link
Member Author

lunny commented Sep 28, 2020

@zeripath Merged.

@lafriks
Copy link
Member

lafriks commented Sep 28, 2020

@lunny backend lint fails

@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 Sep 28, 2020
@zeripath
Copy link
Contributor

Why did you decide to prestash the storage configs here:

func getStorage(sec *ini.Section) Storage {
	var storage Storage
	storage.Type = sec.Key("STORAGE_TYPE").MustString(LocalStorageType)
	storage.Section = sec
	storage.ServeDirect = sec.Key("SERVE_DIRECT").MustBool(false)
	sec.Key("MINIO_ENDPOINT").MustString("localhost:9000")
	sec.Key("MINIO_ACCESS_KEY_ID").MustString("")
	sec.Key("MINIO_SECRET_ACCESS_KEY").MustString("")
	sec.Key("MINIO_BUCKET").MustString("gitea")
	sec.Key("MINIO_LOCATION").MustString("us-east-1")
	sec.Key("MINIO_USE_SSL").MustBool(false)
	return storage
}

func newStorageService() {
	sec := Cfg.Section("storage")
	storages["default"] = getStorage(sec)

	for _, sec := range Cfg.Section("storage").ChildSections() {
		name := strings.TrimPrefix(sec.Name(), "storage.")
		if name == "default" || name == LocalStorageType || name == MinioStorageType {
			log.Error("storage name %s is system reserved!", name)
			continue
		}
		storages[name] = getStorage(sec)
	}
}

Instead of following the pattern of

func GetStorageSettings(name string) { ... }

?

@lunny
Copy link
Member Author

lunny commented Sep 29, 2020

@zeripath because these configurations maybe inherit or override.

@lunny
Copy link
Member Author

lunny commented Sep 29, 2020

@zeripath Could you send your PR directly to gitea once this merged?

@lafriks lafriks merged commit 3878e98 into go-gitea:master Sep 29, 2020
@zeripath
Copy link
Contributor

looks like I'll have to

@lunny lunny deleted the lunny/storage_config branch November 18, 2020 04:37
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 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/bug type/refactoring Existing code has been cleaned up. There should be no new functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants