Skip to content

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

Merged
merged 8 commits into from
Feb 13, 2025
Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Feb 7, 2025

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.

  • The base provider doesn't assume that we are always using an oauth token, that's an implementation detail of each provider. We also no longer directly depend on allauth from the base provider.
  • Every provider implements the operations, no more checks for specific providers (e.g checking for BB when sending a build status).

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.
@stsewd stsewd marked this pull request as ready for review February 10, 2025 17:37
@stsewd stsewd requested a review from a team as a code owner February 10, 2025 17:37
@stsewd stsewd requested a review from humitos February 10, 2025 17:37
@humitos
Copy link
Member

humitos commented Feb 11, 2025

I'm marking this as blocked since I understand we need #11942 first.

@humitos humitos added the Status: blocked Issue is blocked on another issue label Feb 11, 2025
@stsewd
Copy link
Member Author

stsewd commented Feb 11, 2025

@humitos no, this change works without the other PR, it's ready for review.

@stsewd stsewd removed the Status: blocked Issue is blocked on another issue label Feb 11, 2025
Copy link
Member

@ericholscher ericholscher left a 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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

@ericholscher ericholscher left a 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 👍

@stsewd stsewd merged commit 2c963f1 into main Feb 13, 2025
8 checks passed
@stsewd stsewd deleted the refactor-services branch February 13, 2025 21:36
stsewd added a commit that referenced this pull request Feb 14, 2025
- 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants