Skip to content

Conversation

@lunny
Copy link
Member

@lunny lunny commented Aug 18, 2020

As a next step of #11387

@lunny lunny added the type/enhancement An improvement of existing functionality label Aug 18, 2020
@lunny lunny added this to the 1.13.0 milestone Aug 18, 2020
@lunny lunny mentioned this pull request Aug 18, 2020
3 tasks
@lunny lunny force-pushed the lunny/avatar_bucket branch from 47b3ce0 to 0b8cecc Compare August 18, 2020 09:21
@lunny lunny changed the title WIP: Avatar support storing in minio WIP: Avatars and Repo avatars support storing in minio Aug 18, 2020
@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2020

Codecov Report

Merging #12516 into master will decrease coverage by 0.03%.
The diff coverage is 33.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12516      +/-   ##
==========================================
- Coverage   42.59%   42.56%   -0.04%     
==========================================
  Files         671      674       +3     
  Lines       73625    73724      +99     
==========================================
+ Hits        31363    31380      +17     
- Misses      37183    37253      +70     
- Partials     5079     5091      +12     
Impacted Files Coverage Δ
cmd/migrate_storage.go 0.00% <0.00%> (ø)
models/migrations/v115.go 0.00% <0.00%> (ø)
models/repo.go 56.83% <0.00%> (+1.67%) ⬆️
models/repo_generate.go 12.50% <0.00%> (ø)
routers/repo/setting.go 14.69% <0.00%> (ø)
routers/user/setting/profile.go 40.93% <0.00%> (ø)
routers/routes/routes.go 85.86% <23.68%> (-3.14%) ⬇️
models/user_avatar.go 27.47% <27.47%> (ø)
models/repo_avatar.go 31.81% <31.81%> (ø)
models/org.go 73.75% <33.33%> (+0.14%) ⬆️
... and 20 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 c6da033...b419c9f. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 18, 2020
@lunny lunny force-pushed the lunny/avatar_bucket branch from f916357 to 29d6101 Compare August 19, 2020 02:01
@lunny lunny changed the title WIP: Avatars and Repo avatars support storing in minio Avatars and Repo avatars support storing in minio Aug 19, 2020
@lunny
Copy link
Member Author

lunny commented Aug 19, 2020

It's ready to review.

@lunny lunny force-pushed the lunny/avatar_bucket branch from e0889ae to 117209d Compare September 1, 2020 06:13
@lunny lunny force-pushed the lunny/avatar_bucket branch from 117209d to 7abffd6 Compare September 9, 2020 01:29
@lafriks
Copy link
Member

lafriks commented Sep 10, 2020

Imho there should be single avatar storage for both user and repo avatars

@zeripath
Copy link
Contributor

I still think a common [storage] ini section would have been cleaner than spaffing MINIO configuration options in to the configuration for everything you're adding storage to.

@lunny
Copy link
Member Author

lunny commented Sep 11, 2020

Imho there should be single avatar storage for both user and repo avatars

I think we can do that via setting the same configuration for user avatars and repository avatars.

@zeripath
Copy link
Contributor

zeripath commented Sep 11, 2020

We now have to describe both minio and local storage settings in cheat sheet and app.example.ini:

  • [attachment]
  • [lfs]
  • [avatar] (twice)

With exactly the same text and meaning for slightly different keys.

If we add a new storage option the code situation is even worse: as every time we add a new storage option we need to change:

  • modules/setting/lfs.go
  • modules/setting/attachment.go
  • modules/setting/picture.go (twice)
  • cmd/migrate_storage.go
  • modules/storage/storage.go
  • routers/routes/routes.go (twice)
  • Then add descriptions to config cheat sheet and app.example.ini in all of the above places and anywhere else we add storage options to.

Compare with adding a new queue option:

  • add a file in modules/queue
  • add a const type:
// ImmediateType is the type to execute the function when push
const ImmediateType Type = "immediate"
  • add a Queue type:
// Immediate represents an direct execution queue
type Immediate struct {
	handler HandlerFunc
}

...
  • add a creation function:
// NewImmediate creates a new false queue to execute the function when push
func NewImmediate(handler HandlerFunc, opts, exemplar interface{}) (Queue, error) {
	return &Immediate{
		handler: handler,
	}, nil
}
  • add an init function:
func init() {
	queuesMap[ImmediateQueueType] = NewImmediateQueue
}
  • Describe it once in config-cheat-sheet.md and in app.example.ini <- this is missing for the immediate queue type it appears.

Every place that uses queues can now use the immediate queue without having to think about it further.

@lunny
Copy link
Member Author

lunny commented Sep 11, 2020

see #12813

@lafriks
Copy link
Member

lafriks commented Sep 29, 2020

@lunny please resolve conflicts

@lunny lunny force-pushed the lunny/avatar_bucket branch from 222e0a0 to ce6980d Compare September 29, 2020 09:46
@lunny
Copy link
Member Author

lunny commented Sep 29, 2020

@lafriks done.

@zeripath
Copy link
Contributor

I really think we should get #12978 in as it would show a way to simplify the settings somewhat.

@lunny lunny force-pushed the lunny/avatar_bucket branch 2 times, most recently from 2748df7 to b419c9f Compare October 1, 2020 11:35
@lunny lunny force-pushed the lunny/avatar_bucket branch from b419c9f to 464cc66 Compare October 2, 2020 14:07
@lunny lunny force-pushed the lunny/avatar_bucket branch from f67afea to db40c19 Compare October 14, 2020 11:39
@lunny
Copy link
Member Author

lunny commented Oct 14, 2020

@zeripath done.

@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 Oct 14, 2020
@lunny lunny added type/feature Completely new functionality. Can only be merged if feature freeze is not active. and removed type/enhancement An improvement of existing functionality labels Oct 14, 2020
@6543
Copy link
Member

6543 commented Oct 14, 2020

🚀

@lunny lunny merged commit 80a6b0f into go-gitea:master Oct 14, 2020
@lunny lunny deleted the lunny/avatar_bucket branch October 14, 2020 13:07
@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/feature Completely new functionality. Can only be merged if feature freeze is not active.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants