Skip to content

Conversation

@iamrajjoshi
Copy link
Collaborator

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

@codecov
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
@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.

3 participants