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

🐛 fix(azure devops): fix migration version on a fresh install #83290

Merged
merged 4 commits into from
Jan 13, 2025

Conversation

iamrajjoshi
Copy link
Member

@iamrajjoshi iamrajjoshi commented Jan 11, 2025

when the integration is freshly installed, we need to make sure the migration version number is set to 1 as we use this metadata to determine what method we will use to refresh the tokens.

i wrote tests to make sure we refresh correctly when a new integration is installed as well.

to completely fix the bug, we need to do a backfill for all new integrations to update the metadata so we can grab the refresh correctly.

@iamrajjoshi iamrajjoshi self-assigned this Jan 11, 2025
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Jan 11, 2025
Copy link

codecov bot commented Jan 11, 2025

❌ 3 Tests Failed:

Tests completed Failed Passed Skipped
23479 3 23476 272
View the top 3 failed tests by shortest run time
tests.sentry.integrations.vsts.test_client.VstsApiClientTest::test_refreshes_expired_token
Stack Traces | 3.4s run time
#x1B[1m#x1B[.../integrations/vsts/test_client.py#x1B[0m:54: in test_refreshes_expired_token
    assert responses.calls[-2].request.url == "https://app.vssps.visualstudio.com/oauth2/token"
#x1B[1m#x1B[31mE   AssertionError: assert 'https://logi...h2/v2.0/token' == 'https://app..../oauth2/token'#x1B[0m
#x1B[1m#x1B[31mE     #x1B[0m
#x1B[1m#x1B[31mE     - https://app.vssps.visualstudio.com/oauth2/token#x1B[0m
#x1B[1m#x1B[31mE     + https://login.microsoftonline..../common/oauth2/v2.0/token#x1B[0m
tests.sentry.integrations.vsts.test_integration.VstsIntegrationTest::test_get_repositories_identity_error
Stack Traces | 3.6s run time
#x1B[1m#x1B[.../integrations/vsts/test_integration.py#x1B[0m:616: in test_get_repositories_identity_error
    with pytest.raises(IntegrationError):
#x1B[1m#x1B[31mE   Failed: DID NOT RAISE <class 'sentry.shared_integrations.exceptions.IntegrationError'>#x1B[0m
tests.sentry.integrations.vsts.test_integration.VstsIntegrationTest::test_get_organization_config_failure
Stack Traces | 3.66s run time
#x1B[1m#x1B[.../integrations/vsts/test_integration.py#x1B[0m:464: in test_get_organization_config_failure
    assert fields[0]["disabled"], "Fields should be disabled"
#x1B[1m#x1B[31mE   AssertionError: Fields should be disabled#x1B[0m
#x1B[1m#x1B[31mE   assert False#x1B[0m

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

@iamrajjoshi iamrajjoshi marked this pull request as ready for review January 13, 2025 17:10
@iamrajjoshi iamrajjoshi requested review from a team as code owners January 13, 2025 17:10
Copy link
Contributor

@Christinarlong Christinarlong left a comment

Choose a reason for hiding this comment

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

Q: who will go under this feature flag ?

@iamrajjoshi iamrajjoshi requested a review from a team January 13, 2025 17:58
@iamrajjoshi
Copy link
Member Author

Q: who will go under this feature flag ?

we already use this ff to gate if we use the new devops integrations or not. though its currently ga'ed, using the flag will make rollback easier if needed. we will eventually remove this flag

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: Backend Automatically applied to PRs that change backend components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants