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

ref(messaging): Removing Integration Namespace Imports #72033

Merged
merged 5 commits into from
Jun 4, 2024

Conversation

iamrajjoshi
Copy link
Member

Importing the entire integration module is bad practice and it turns out we only do it to access the IntegrationManager. I moved the instantiation of the manager back to the original file and import the manager directly.

@iamrajjoshi iamrajjoshi self-assigned this Jun 4, 2024
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jun 4, 2024
@iamrajjoshi iamrajjoshi requested a review from a team June 4, 2024 18:46
@iamrajjoshi iamrajjoshi marked this pull request as ready for review June 4, 2024 18:46
@iamrajjoshi iamrajjoshi requested review from a team as code owners June 4, 2024 18:46
@iamrajjoshi iamrajjoshi removed the request for review from a team June 4, 2024 18:46
Comment on lines 17 to 20
def get_provider(instance: Integration | RpcIntegration) -> IntegrationProvider:
from sentry import integrations
from sentry.integrations.manager import default_manager as integrations

return integrations.get(instance.provider)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably a sign that this method doesn't belong here, since the import should be happening the other way around. Or, if anything, move this to a util file under manager

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.91%. Comparing base (27a2f5e) to head (9792369).
Report is 15 commits behind head on master.

Current head 9792369 differs from pull request most recent head 8aa1f2a

Please upload reports for the commit 8aa1f2a to get more accurate results.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #72033   +/-   ##
=======================================
  Coverage   77.91%   77.91%           
=======================================
  Files        6573     6573           
  Lines      292707   292751   +44     
  Branches    50523    50527    +4     
=======================================
+ Hits       228059   228100   +41     
- Misses      58401    58406    +5     
+ Partials     6247     6245    -2     
Files Coverage Δ
src/sentry/api/endpoints/group_integrations.py 94.00% <100.00%> (+0.12%) ⬆️
src/sentry/api/endpoints/integrations/index.py 93.33% <100.00%> (+0.22%) ⬆️
...ntry/api/endpoints/integrations/install_request.py 87.50% <100.00%> (ø)
.../sentry/api/endpoints/project_repo_path_parsing.py 100.00% <100.00%> (ø)
src/sentry/integrations/__init__.py 100.00% <ø> (ø)
src/sentry/integrations/manager.py 76.92% <100.00%> (+4.19%) ⬆️
src/sentry/integrations/pipeline.py 93.80% <100.00%> (ø)
src/sentry/models/integrations/utils.py 76.92% <100.00%> (ø)
src/sentry/notifications/utils/__init__.py 73.86% <100.00%> (ø)
src/sentry/testutils/pytest/sentry.py 84.57% <100.00%> (ø)
... and 1 more

... and 12 files with indirect coverage changes

@iamrajjoshi iamrajjoshi enabled auto-merge (squash) June 4, 2024 20:21
@iamrajjoshi iamrajjoshi merged commit 25c760f into master Jun 4, 2024
48 checks passed
@iamrajjoshi iamrajjoshi deleted the raj/mes-ref/integration-imports branch June 4, 2024 20:31
@github-actions github-actions bot locked and limited conversation to collaborators Jun 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants