-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Git service: depend on the project instead of users #11983
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
Conversation
With #11942, a GH app is used to interact with the GH API, a GH app is not directly tied to a user, so this assumption is not valid anymore. For most of the operations (except for syncing repos), we don't need to know who is the user that is performing the action, we just need to know the project that is being affected.
I'm marking this as blocked since I understand we need #11942 first. |
@humitos no, this change works without the other PR, it's ready for review. |
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 looks like a solid refactor -- I'm surprised more tests didn't need to get updated
return provider_class.name | ||
return None | ||
service_class = self.get_git_service_class(fallback_to_clone_url=True) | ||
return service_class.provider_name if service_class else None |
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.
Should this return an empty string instead of None if it's used to display it?
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.
Doesn't look like this is being used for display (not directly), but to identify the type of provider... so this can probably be removed and callers should use get_git_service_class instead.
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.
Will do that in another PR, as returning none matches the current behavior.
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.
These changes look good 👍
- With #11942, there is the need to get an OAuth2 client from an account, so that method was moved outside the UserService class, and it just depends on the social account. - provider_id and provider name don't need to be an instance method, and can be extracted from the allauth provider attached to the service class. This is on top of #11983
With #11942, a GH app is used to interact with the GH API,
a GH app is not directly tied to a user, so this assumption is not valid anymore.
For most of the operations (except for syncing repos), we don't need to know who is the user that is performing the action, we just need to know the project that is being affected.