-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Update OAuth App to Use The RemoteRepositoryRelation Model #7675
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
Update OAuth App to Use The RemoteRepositoryRelation Model #7675
Conversation
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.
Changes look GOOD! I left some comments to keep talking, but there isn't anything too big to change.
This does not take into account for duplicate RemoteRepository for now.
Is this comment old now? The code seems to handle duplicated RemoteRepository properly now.
@@ -155,15 +165,18 @@ def create_repository(self, fields, privacy=None, organization=None): | |||
|
|||
repo.html_url = fields['links']['html']['href'] | |||
repo.vcs = fields['scm'] | |||
repo.account = self.account |
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.
It may be good to start populating remote_id
here as well. I think we already added to the modeling.
It will be easier to populate it first, and after that do the switch to start using 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.
This comment is valid for all the services we support.
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.
Actually, I think we talked about adding the remote_id
fields later, but now that I think of it it would be better to do it now, but I'll add it in another PR after this is merged.
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. I said that in our conversation. However, I think it's better to have it before we do the initial resync so we already populate this data from the beginning and then we can start making use of it. I'm fine if it's in another PR.
Unfortunately, it does not handle it. We can only filter repo, _ = RemoteRepository.objects.get_or_create(
full_name=fields['full_name']
) We need to figure out a way to remove the current DB duplicates |
I understand. We may not need to care about this anymore, if the what I'm going to describe next makes sense 😅 Talking to Eric last week, we thought that we can deploy all of these changes in a slightly different way than we originally described in #7536 (comment):
|
Sounds good 💯 |
2c74027
to
2df6a4e
Compare
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 re-review this PR and everything looks fine to me. Thanks for your help on this! We can now move forward with the similar changes required for RemoteOrganization
] | ||
|
||
operations = [ | ||
migrations.RunPython(move_data_to_remote_relations), | ||
migrations.RunPython(move_data_to_remote_repository_relations), |
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 won't probably be required in the end as we may resync everything (pull down from VCS providers) a week in advance, but we can create a note and remove it later if needed.
This does not take into account for duplicate RemoteRepository for now.