Skip to content
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

Grab the correct name of RemoteOrganization to use in the query #7430

Merged
merged 9 commits into from Sep 8, 2020

Conversation

humitos
Copy link
Member

@humitos humitos commented Sep 1, 2020

We were using the naked response from GitHub to exclude RemoteOrganization
objects and delete the ones where the user don't have permissions anymore.
However, we need the organization details instead that comes with the name
attribute which is the one that we use to create the RemoteOrganization object.

I refactored this code to always return a list of RemoteRepository and RemoteOrganization from the sync_repositories and sync_organizations methods on each service to avoid using the wrong field name, as we were doing.

Closes #7381

We were using the naked response from GitHub to exclude `RemoteOrganization`
objects and delete the ones where the user don't have permissions anymore.
However, we need the organization details instead that comes with the `name`
attribute which is the one that we use to create the `RemoteOrganization` object.
Copy link
Member

@ericholscher ericholscher left a comment

Seems this could use a test

humitos added 4 commits Sep 2, 2020
Instead of returning a list of dictionaries with different structure, we are
returning a list of RemoteRepository and RemoteOrganization. Then, we use the
same field names (`full_name` and `name`) from the objects we are going to
filter by the old values to be removed.

This reduces the complexity of choosing the right field for each service from
the dictionary previously returned; avoiding picking the wrong field, returning
`None` and removing all the Remote* objects for that user.
- `sync_repositories` creates RemoteRepository from API
- `sync_organizations` creates RemoteOrganization from API
- `sync` removes stale RemoteRepository and RemoteOrganization
  - do not remove RemoteRepository if linked to a Project
  - do not remove RemoteRepository where the user still have permissions
@humitos humitos requested a review from ericholscher Sep 2, 2020
@@ -25,3 +25,5 @@ pytest-mock==3.3.0
# local debugging tools
datadiff==2.0.0
ipdb==0.13.3

requests-mock==1.8.0
Copy link
Member

@stsewd stsewd Sep 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: now we can remove this dep from .com

stsewd
stsewd approved these changes Sep 3, 2020
We were using the naked response from GitHub to exclude `RemoteOrganization`
objects and delete the ones where the user don't have permissions anymore.
However, we need the organization details instead that comes with the `name`
attribute which is the one that we use to create the `RemoteOrganization` object.
@stsewd stsewd force-pushed the humitos/sync-orgs-github branch from c49478e to 12d0a52 Compare Sep 3, 2020
@stsewd stsewd added the PR: hotfix Pull request applied as hotfix to release label Sep 3, 2020
@ericholscher
Copy link
Member

ericholscher commented Sep 4, 2020

This looks good once we fix the conflict. 👍

humitos added 3 commits Sep 8, 2020
`name` is not mandatory and some organization does not have it. That's why we
were deleting some valid organizations.
@humitos
Copy link
Member Author

humitos commented Sep 8, 2020

@stsewd be careful with force-push next time. You deleted a commit that added some tests.

@humitos humitos merged commit c3eee5d into master Sep 8, 2020
2 checks passed
@humitos humitos deleted the humitos/sync-orgs-github branch Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: hotfix Pull request applied as hotfix to release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Organization not showing up in repository list
3 participants