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

Stop swallowing errors when paginating #2903

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

seanstory
Copy link
Member

Closes #2394

Previously, paginated_api_call would swallow all exceptions (after logging a warning). Now, it raises, them, and let's the caller decide whether to swallow or re-raise them.

  • _remote_validation(): re-raises (seems important to fail validation if invalid)
  • fetch_attachments(): logs warnings, but does not raise
  • fetch_page_blog_documents(): logs warnings, but does not raise
  • fetch_spaces(): re-raises (seems important to not skip whole spaces - that should fail the sync)
  • search_by_query(): re-raises (seems important to let you know if your advanced sync rules are incorrect)

Also, I took a pass through the file to make sure that we were using exc_info=exception everywhere we were logging about an exception, instead of stuff like f"Exception: {exception}"

Checklists

Pre-Review Checklist

  • this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check config.yml.example)
  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)

Related Pull Requests

Release Note

Improved the Confluence connector by ensuring that Confluence pagination API request errors bubble up when appropriate. This may result in more syncs ending in an error state, but these would not be new errors/issues, they will just be more visible and actionable.

@seanstory
Copy link
Member Author

CI failed. Separate fix is: #2905
Will rebuild after that merges

@artem-shelkovnikov
Copy link
Member

Shall we merge?

@seanstory
Copy link
Member Author

I didn't write tests. I'd asked Crest to, but now I think it's back on us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

confluence swallows any error that occurs during pagination
2 participants