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

Fix #1334: add retry + sleep to github team-repo sync #1336

Merged
merged 5 commits into from
Jul 22, 2024
Merged

Conversation

achantavy
Copy link
Contributor

@achantavy achantavy commented Jul 22, 2024

Summary

Describe your changes.

Adds a retry with sleep to _get_team_repos_for_multiple_teams because we have seen the GitHub GraphQL API sometimes return None for fields that are not supposed to be None.

Related issues or links

Include links to relevant issues or other pages.

Proof that this works

We can merge your change in faster if we see that it works. For example, if making a change to the graph, include a
screenshot showing what the graph looked like before and after your changes. You can also include console log traces
showing what happened before and after your changes.

Added unit tests.

Checklist

  • Update/add unit or integration tests

If you are modifying or implementing an intel module:

@achantavy achantavy marked this pull request as ready for review July 22, 2024 06:22
try:
# The `or []` is because `.nodes` can be None. See:
# https://docs.github.com/en/graphql/reference/objects#teamrepositoryconnection
for repo in team_repos.nodes or []:
Copy link
Contributor

Choose a reason for hiding this comment

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

Trying to understand the Python magic here.
If team_repos.nodes == None then the for condition will break before assigning any element from the iterable to repo?

Copy link
Contributor Author

@achantavy achantavy Jul 22, 2024

Choose a reason for hiding this comment

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

If team_repos.nodes is None, then the for loop will iterate through the empty list. See this:

>>> derp = {'a': [1,2,3], 'b': None}
>>> for r in derp['a'] or []:
...   print(r)
...
1
2
3
>>> for r in derp['b'] or []:
...   print(r)
...
# prints nothing

heryxpc
heryxpc previously approved these changes Jul 22, 2024
@achantavy achantavy merged commit 8c05980 into master Jul 22, 2024
5 checks passed
@achantavy achantavy deleted the repopermsfix branch July 22, 2024 18:09
SecPrez pushed a commit to SecPrez/cartography that referenced this pull request Nov 10, 2024
…cartography-cncf#1336)

### Summary
> Describe your changes.

Adds a retry with sleep to `_get_team_repos_for_multiple_teams` because
we have seen the GitHub GraphQL API sometimes return None for fields
that are not supposed to be None.

### Related issues or links
> Include links to relevant issues or other pages.

- cartography-cncf#1334


### Proof that this works
> We can merge your change in faster if we see that it works. For
example, if making a change to the graph, include a
> screenshot showing what the graph looked like before and after your
changes. You can also include console log traces
> showing what happened before and after your changes.

Added unit tests.


### Checklist

- [x] Update/add unit or integration tests

If you are modifying or implementing an intel module:
- [ ] Update the
[schema](https://github.com/lyft/cartography/tree/master/docs/root/modules)
and
[readme](https://github.com/lyft/cartography/blob/master/docs/schema/README.md)
- [ ] Use the NodeSchema [data
model](https://lyft.github.io/cartography/dev/writing-intel-modules.html#defining-a-node)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants