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

Logged errors in Github repo sync need more context #815

Open
achantavy opened this issue Apr 15, 2022 · 2 comments
Open

Logged errors in Github repo sync need more context #815

achantavy opened this issue Apr 15, 2022 · 2 comments
Labels
stale stalebot believes this issue/PR is no longer active

Comments

@achantavy
Copy link
Contributor

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.

I am seeing error logs that look like

[{'type': 'FORBIDDEN', 'path': ['organization', 'repositories', 'nodes', 93, 'collaborators'], 'locations': [{'line': 43, 'column': 21}], 'message': 'You do not have permission to view repository collaborators.'}]

I have confirmed that my Github PAT does have permission to view repository collaborators and as far as I can tell the sync actually runs to completion so I'm confused here.

In any case,

  • The log message should make it clear which asset is failing to be retrieved.
  • Since we don't currently treat this code path as a fatal error, we should consider reducing the log level to warning, or we could make it a fatal error.

To Reproduce:

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

Run github sync with a PAT that does have permission to view collaborators on a repo that has external collaborators.

Please complete the following information::

  • Cartography release version or commit hash [e.g. 0.12.0 or 95e8e11]
    0.57.0rc1

cc: @ryan-lane

@ryan-lane
Copy link
Collaborator

Well, unfortunately, there's a lot to unpack here. It could be a fatal error, or just a warning, depending. Since we're using the graphql API, it could be returning partial (but usable) results, or it could be returning nothing. We're seeing this now because previously, the response would be missing data and we'd raise an exception about the missing data. graphql puts the error message into the 200 response, so I exposed that error as a logged error so we could see why the query was failing.

I think switching it to a warning is fine, but I think in general, if anything is shown, it probably means you're missing some data that the module supports.

@stale
Copy link

stale bot commented May 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale stalebot believes this issue/PR is no longer active label May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale stalebot believes this issue/PR is no longer active
Projects
None yet
Development

No branches or pull requests

2 participants