-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Re-sync GitHub (RemoteRepository and RemoteOrganization) on webhook #7336
Conversation
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This PR is useful. I haven't had the time to update it with the newer code, but we will eventually need it. |
Take into account that this only works for repository collaborators. There is another webhook called Even if we merge this PR, all the Read the Docs projects have to re-sync their webhooks (:disappointed: ) to make usage of the new |
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'd agree that celery task is the most correct here. We've had issues lately with blocking web processes and should avoid that pattern going forward.
We are moving away from OAuth Application to GitHub Application: #8445 If anything, we will implement the re-sync via the events we can subscribe there. |
I'd love to get this working via Celery. I think it might help us keep SSO in sync. |
3a95a08
to
aafe258
Compare
I updated this PR to use a Celery task to sync but... Note this PR does not cover exactly what we want, and I'm not sure if is worth merging it and maintaining it:
Taking into account all of these points, and considering that we added a scheduled Celery task to re-sync in #8601 it may not make sense to merge it. |
@humitos I don't fully understand the issues with it, but I do think we want some kind of way to do real-time updates on permissions. Having permissions be out of sync for users for a whole day feels like bad UX, so I feel like we need to at least attempt to do this. |
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 think this makes sense, and doesn't seem like a lot of code to maintain?
Add a Django Admin Action under `SSOIntegration` to trigger one `sync_remote_repositories` task per each user member of the organization. This allows us to easily re-sync `RemoteRepository` and `RemoteRepositoryRelation` for the whole organization.
Add `member` event when creating the Read the Docs' webhook on each repository imported under Read the Docs. This webhook will communicate back to us when a collaborator was added/removed/changed from that repository in particular. This allow us to trigger the re-sync method for the GitHub service on that user and create/delete/change `RemoteRepository` objects for that user. Examples, * user is removed from having access to a repository -> remove RemoteRepository * user is added access to a repository -> create RemoteRepository * user is removed admin access but keep read access -> `.admin=False`
53226cd
to
05904e1
Compare
05904e1
to
423347b
Compare
The implementation of this webhook may make you feel that it solves the problem, where it does not. That's what I'm trying to say. It will keep permissions over repositories updated, but it won't for permissions granted via organizations and teams --which is the main problem we want to solve here. That said, it may help in some cases and add more confusion in others. The bad UX because we update permissions once a day, will still be present even with this webhook. |
@humitos Good points. I think I'm OK leaving this out for now if we want, and just doing nightly updates. That seems cleanest and easiest to explain for now, until we can find a better solution. I agree this does add a good bit of confusion, and "it might work" situations, which is probably not great. |
Closing this again... Seems we were correct the first time 🙃 |
Add
member
event when creating the Read the Docs' webhook on each repositoryimported under Read the Docs.
This webhook will communicate back to us when a collaborator was
added/removed/changed from that repository in particular. This allow us to
trigger the re-sync method for the GitHub service on that user and
create/delete/change
RemoteRepository
objects for that user.Examples,
.admin=False
Related to #7185
Documentation reference: https://docs.github.com/en/developers/webhooks-and-events/webhook-events-and-payloads#member