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

Disqus avatars - straightforward approach #1513

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

VladimirZaets
Copy link

@VladimirZaets VladimirZaets commented Oct 15, 2022

The most straightforward fix for avatars import ( #990 ) mostly to start discussion of possible approaches.

I don't like current fix but I don't see another way without code duplication or refactoring of auth package.

The best way that I see it's split backend/vendor/github.com/go-pkgz/auth/avatar/avatar.go file to 2 structs, Proxy and AvatarSaver.

First of all, because they have different responsibility, Proxy works with paths, and AvatarSaver should declare rules how to save avatar. Even if we will look use cases, proxy and AvatarSaver parts uses in different scenarios.

AvatarSaver can use Proxy to save avatar, but it shouldn't be same struct.
Also will needed to provide possibility to pass Proxy and AvatarSaver instances as parameters of auth package. If talk about AvatarSaver maybe we need provide possibility to pass it in runtime (as a param to save method).

This approach will provide possibility to declare different save mechanism for different providers.
Each third party api can have different ratelimits, etc, and maybe even different protocols, not http. With this approach we will be able to configure it specifically for each API.

Will be glad to hear other ideas.

@paskal paskal added this to the v1.11.1 milestone Oct 20, 2022
@paskal paskal added the backend label Oct 20, 2022
@paskal paskal modified the milestones: v1.11.1, v1.11.2 Dec 2, 2022
@paskal paskal modified the milestones: v1.11.2, v1.11.3, v1.12.0 Jan 3, 2023
@paskal paskal removed this from the v1.12.0 milestone Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants