-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(ACI): Fix adding sentry app action to a workflow #103790
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
base: master
Are you sure you want to change the base?
Conversation
❌ 7 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| "sentryAppIdentifier": SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID, | ||
| "targetIdentifier": self.sentry_app_installation.uuid, | ||
| "targetType": ActionType.SENTRY_APP, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ameliahsu I was only able to get this test to pass if I used this data which differs from what was passed in https://sentry.sentry.io/issues/6980690322 which was
{
"sentry_app_identifier":"'sentry_app_id'",
"target_identifier":"'72013'",
"target_type":"'sentry_app'"
}
Do you know if the data in the Sentry issue is what's expected to be passed all the time or only sometimes? I'm not sure when we pass one versus the other, but if we set sentryAppIdentifier to "sentry_app" instead of "sentry_app_installation" (SentryAppIdentifier.SENTRY_APP_INSTALLATION_UUID) we go down a slightly different code path and set sentryAppInstallationUuid to None (code here) which leads to a SentryAppInstallation.DoesNotExist error (here).
I truly don't know if this is a bug or how it's intended to work, but it doesn't currently work if you do not pass the installation data. I did verify that even an internal unpublished sentry app has an installation object, so maybe it's intended for us to always pass that data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GabeVillalobos might be able to help here, i believe some existing app actions use the sentry app UUID instead of the sentry app ID, which is why we have to specify sentry_app_identifier? but we would like for all newly created actions going forward to use sentry app ID. not sure if that's why we throw an error tho 😅
|
I'm finding that I'm between a rock and a hard place because the old alert rule tests are failing when we dual write to ACI models and don't pass Update - I ran into this test with no settings failing and I don't know if this is supposed to be valid or not. |
| "properties": { | ||
| "settings": {"type": ["array", "object"]}, | ||
| }, | ||
| "required": ["settings"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I no longer feel confident this should be required - we have actions in the db without data. Are those invalid? If so what should we do about them?
Fixes adding sentry app actions to a workflow and requires
settingsto be passed for a sentry app action if it has a schema. If it does not have a schema,settingsdoes not need to be passed. Add tests for adding and updating sentry app actions.Fixes https://sentry.sentry.io/issues/6980690322
Do not merge until we can run the migration to move
sentry_app_installation_uuid->sentry_app_id(or support both).