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.

Sign up for GitHub

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

Jump to bottom

Unify hashing for avatar #22289

Merged
merged 8 commits into from
Jan 2, 2023
Merged

Unify hashing for avatar #22289

merged 8 commits into from
Jan 2, 2023

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Dec 31, 2022

  • Unify the hashing code for repository and user avatars into a function.
  • Use a sane hash function instead of MD5.
  • Only require hashing once instead of twice(w.r.t. hashing for user avatar).
  • Improve the comment for the hashing code of why it works.

@Gusted
Unify hashing for avatar
33cc882 
- Unify the hashing code for repo and user avatars into a function.
- Use a sane hash function instead of MD5.
- Only require hashing instead of twice(w.r.t. hashing for user avatar).
- Improve the comment for the hashing code of why it works.
@Gusted
Merge branch 'main' into easier-avatar-hash
d4ff820
@Gusted
Fix typo
77a31ab
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 31, 2022
delvh
delvh approved these changes Jan 1, 2023
View reviewed changes
Copy link
Member

@delvh delvh left a comment

Choose a reason for hiding this comment

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

We only change the URL where new avatars are stored with this PR, right?
So, existing avatars are unaffected?

@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 Jan 1, 2023
@delvh delvh added type/refactoring Existing code has been cleaned up. There should be no new functionality. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jan 1, 2023
@lunny @yardenshoham
Update modules/avatar/hash.go
cfb6b3c 
Co-authored-by: Yarden Shoham <hrsi88@gmail.com>
@lunny
Copy link
Member

lunny commented Jan 1, 2023

/drone/src/modules/avatar/hash.go:9:2: "fmt" imported but not used

yardenshoham
yardenshoham reviewed Jan 1, 2023
View reviewed changes
modules/avatar/hash.go Outdated Show resolved Hide resolved
lunny and others added 2 commits January 2, 2023 00:16
@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 Jan 1, 2023
@delvh
Copy link
Member

delvh commented Jan 1, 2023 

--- FAIL: TestUploadAvatar (0.15s)

    avatar_test.go:31: 

        	Error Trace:	/drone/src/services/repository/avatar_test.go:31
        	Error:      	Not equal: 
        	            	expected: "10-9e24e19b024c44b778301d880bd8e6f4"
        	            	actual  : "d5fcd714cb1265ec33652f2d41c48b7f42a8fc1d282b8c577b3e997d206ab4b6"

        	            	Diff:
        	            	--- Expected
        	            	+++ Actual
        	            	@@ -1 +1 @@
        	            	-10-9e24e19b024c44b778301d880bd8e6f4
        	            	+d5fcd714cb1265ec33652f2d41c48b7f42a8fc1d282b8c577b3e997d206ab4b6
        	Test:       	TestUploadAvatar

FAIL

I guess that failure makes sense?

@Gusted
Copy link
Contributor Author

Gusted commented Jan 2, 2023

We only change the URL where new avatars are stored with this PR, right? So, existing avatars are unaffected?

Yup that's correct.

@KN4CK3R KN4CK3R merged commit 96797fe into go-gitea:main Jan 2, 2023
@Gusted Gusted deleted the easier-avatar-hash branch January 2, 2023 21:49
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 3, 2023
@zjjhot
Merge remote-tracking branch 'upstream/main'
b9d2ea0 
* upstream/main:
  Add deprecated warning for DISABLE_GRAVATAR and ENABLE_FEDERATED_AVATAR (go-gitea#22318)
  Unify hashing for avatar (go-gitea#22289)
  fix: code search title translation (go-gitea#22285)
  Update Gmail mailer configuration (go-gitea#22291)
  Fix due date rendering the wrong date in issue (go-gitea#22302)
  Fix get system setting bug when enabled redis cache (go-gitea#22295)
  Restructure `webhook` module (go-gitea#22256)
  Reminder for no more logs to console (go-gitea#22282)
  Fix bug of DisableGravatar default value (go-gitea#22296)
  Upgrade go-chi to v5.0.8 (go-gitea#22304)
  [skip ci] Updated licenses and gitignores
  Use ErrInvalidArgument in packages (go-gitea#22268)
  Changelog v1.18.0 (go-gitea#22215) (go-gitea#22269)
  Support estimated count with multiple schemas (go-gitea#22276)
  Add Gentoo to the from package providers (go-gitea#22284)
  Fix sitemap (go-gitea#22272)
  Add `sync_on_commit` option for push mirrors api (go-gitea#22271)
  Fix key signature error page (go-gitea#22229)
@astos-marcb astos-marcb mentioned this pull request Mar 9, 2023
Closed
@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.
Reviewers

@yardenshoham yardenshoham yardenshoham left review comments

@lunny lunny lunny approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Gusted @lunny @delvh @yardenshoham @KN4CK3R @GiteaBot