-
Notifications
You must be signed in to change notification settings - Fork 1
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
Simple implementation of profile pic federation #427
Conversation
@@ -18,6 +18,7 @@ services: | |||
- default | |||
environment: | |||
- SKINNY_SERVER_PORT=[[SKINNY_PORT]] | |||
- HOST_NAME=[[SKINNY_HOST]]:[[SKINNY_PORT]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enjoy your merge conflict with aaron
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will
@@ -31,6 +31,7 @@ def _create_actor(self, username): | |||
outbox=None, | |||
following=following_url, | |||
followers=followers_url, | |||
global_id=user.global_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Rabble global IDs are internal-only, why are you making it public here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it's needed for grabbing the profile pic, the format is assets/user_${global_id}
services/utils/users.py
Outdated
directory. This should all really be a webfinger thing but we don't | ||
have that option at the time of writing. | ||
|
||
TODO(CianLR): Erase this function from git history. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this line, erase
is such a negative word. Besides, here at Rabble, we look to the future, not to the past
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this function is all that bad, just the first line of hardcoding a URL is nasty, but that's how we do it right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍, honestly this isn't that bad
# Should not try to create a user and hope it has this ID. | ||
# Also shouldn't create a user with no handle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused when this will happen. Is this just in case someone queries the db and forgets to put a handle
in their call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, pretty much, it's a weird use case that would probably only happen accidentally but it's better than having to debug why there's a user with ""
as a handle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, a few nitpicks
services/utils/users.py
Outdated
directory. This should all really be a webfinger thing but we don't | ||
have that option at the time of writing. | ||
|
||
TODO(CianLR): Erase this function from git history. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍, honestly this isn't that bad
Outbox string `json:"outbox"` | ||
Name string `json:"name"` | ||
PreferredUsername string `json:"preferredUsername"` | ||
Icon *ImageObject `json:"icon,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe this should be called IconObject
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actual activitypub message type is Image, it's just used as Icon here, that was the reasoning anyhow
env := "SKINNY_SERVER_PORT" | ||
port := os.Getenv(env) | ||
host_env := "HOST_NAME" | ||
hostname := os.Getenv(host_env) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way of initializing the hostname is probably better than what I did, so I guess this should be merged before webfinger
Connects to #392
This contains a few bits:
The main limitation of this is that the cached images never expire, I'm not sure where best to implement that or if it's necessary since we plan on fixing this whole hack anyhow. Open to suggestions