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

github sync nonetype error when listing repo-teams #1334

Closed
achantavy opened this issue Jul 20, 2024 · 3 comments
Closed

github sync nonetype error when listing repo-teams #1334

achantavy opened this issue Jul 20, 2024 · 3 comments
Labels
bug Something isn't working GitHub Related to GitHub intel module

Comments

@achantavy
Copy link
Contributor

Bug Template

Description:

What issue is being seen? Describe what should be happening instead of the bug, for example: Cartography should not crash, the expected value isn't returned, the data schema is wrong, etc.

GitHub sync crashes when listing repo-teams with a Nonetype error;

To Reproduce:

Steps to reproduce the behavior. Provide all data and inputs required to reproduce the issue.

Not sure why this suddenly started failing.

Logs:

If applicable, copy and paste your console log with the failing stack trace.

Crash:

Traceback (most recent call last):
  File "PATH/cartography/intel/github/teams.py", line 174, in sync_github_teams
    team_repos = _get_team_repos_for_multiple_teams(teams_paginated.nodes, organization, github_url, github_api_key)
  File "PATH/cartography/util.py", line 192, in timed
    result = method(*args, **kwargs)
  File "PATH/cartography/intel/github/teams.py", line 63, in _get_team_repos_for_multiple_teams
    repo_urls = [t['url'] for t in team_repos.nodes] if team_repos.nodes else []
  File "PATH/cartography/intel/github/teams.py", line 63, in <listcomp>
    repo_urls = [t['url'] for t in team_repos.nodes] if team_repos.nodes else []
TypeError: 'NoneType' object is not subscriptable

Please complete the following information::

  • Cartography release version or commit hash [e.g. 0.12.0 or 95e8e11]
    0.93.0
@achantavy achantavy added bug Something isn't working GitHub Related to GitHub intel module labels Jul 20, 2024
@achantavy
Copy link
Contributor Author

We fail here
https://github.com/lyft/cartography/blob/e149967cd2750c614d0cb94fced6b1e60870a5cf/cartography/intel/github/teams.py#L63

which relies on data from this graphql object:
https://github.com/lyft/cartography/blob/e149967cd2750c614d0cb94fced6b1e60870a5cf/cartography/intel/github/teams.py#L85

and that is defined here:
https://docs.github.com/en/graphql/reference/objects#repository

image

The ! indicates the field is non-nullable, so since it has returned None that means something bad has happened on the GitHub server side and it is raising an exception to give us the option to stop execution.

We could add in logic to continue on despite this GitHub error, but this would cause us to lose team-to-repo mappings or team permission data in the graph. This is because something wrong happened on GitHub's end and if we don't include a team-to-repo mapping in this run, it will get cleaned up and disappear.

I think I'd prefer to continue the current behavior of failing the sync here rather than losing data when GitHub has problems. Still, there may be something wrong with our specific deployment if this problem is happening frequently enough.

cc: @heryxpc

@achantavy
Copy link
Contributor Author

achantavy commented Jul 20, 2024

The change could look something like this:
image

but again, this would mean continuing on with data loss.

@achantavy
Copy link
Contributor Author

Another approach: wrap it with retry + sleep. I started this in https://github.com/lyft/cartography/pull/1336/files. Will add tests when I have time but assuming my theory about GitHub API not behaving well is correct, this should help.

SecPrez pushed a commit to SecPrez/cartography that referenced this issue 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)
chandanchowdhury pushed a commit to chandanchowdhury/cartography that referenced this issue Nov 27, 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)

Signed-off-by: chandanchowdhury <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working GitHub Related to GitHub intel module
Projects
None yet
Development

No branches or pull requests

1 participant