Skip to content

fix(ACI): Re-land GitHub ticket action validation (#114095) with schema and selective test fix#114500

Merged
ceorourke merged 6 commits intomasterfrom
joshuarli/fix/reland-github-ticket-action-validation
May 1, 2026
Merged

fix(ACI): Re-land GitHub ticket action validation (#114095) with schema and selective test fix#114500
ceorourke merged 6 commits intomasterfrom
joshuarli/fix/reland-github-ticket-action-validation

Conversation

@joshuarli
Copy link
Copy Markdown
Member

@joshuarli joshuarli commented Apr 30, 2026

Summary

Re-applies #114095 (reverted in a630f3f due to a CI failure that selective testing had missed)

Original change (re-landed):

  • Adds a data_schema to GithubActionHandler requiring repo in additional_fields, so the API rejects GitHub ticketing actions created without a repo.
  • Makes GithubEnterpriseActionHandler inherit from GithubActionHandler to pick up the same validation.

Schema fix:

  • Removes integration from required in additional_fields. The IssueAlertMigrator translator moves integration into Action.integration_id (a dedicated model field), so it never lands in additional_fields. Requiring it in the schema caused action.save() to raise a ValidationError when serializing existing GitHub/GithubEnterprise rules via WorkflowEngineRuleSerializer.

edit: this doesn't really work so i'm prioritizing getting this PR back in and overriding selective testing for now

Selective testing fix:

  • Adds explicit import sentry.integrations.github.handlers and import sentry.integrations.github_enterprise.handlers at the top of test_rule.py. Both handlers self-register via @action_handler_registry.register at import time (a Django startup side-effect), which means the static import scanner had no edge from the handler files to test_rule.py. The explicit imports make the dependency traceable, so future handler changes will select this test.

… fix

Re-applies 36ce446 (reverted in a630f3f) which requires `repo` and
`integration` to be passed when creating a GitHub ticketing action.

The revert was triggered by a CI failure in test_rule.py that selective
testing had missed: GithubEnterpriseActionHandler is loaded via Django
startup side-effects (@registry.register decorator), so the static
import scanner had no edge from the handler file to the test file.

Fix: add explicit imports of the github and github_enterprise handler
packages at the top of test_rule.py so the scanner can detect the
dependency going forward.

Separately fix: remove `integration` from `required` inside
`additional_fields` in the data_schema — the translator stores it as
Action.integration_id, not inside additional_fields.
The IssueAlertMigrator translator moves `integration` from rule action data
into Action.integration_id (a dedicated model field), so it never appears
in additional_fields. Requiring it in the schema caused a ValidationError
on action.save() when migrating existing GitHub/GithubEnterprise rules.
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 30, 2026
@joshuarli joshuarli marked this pull request as ready for review April 30, 2026 22:04
@joshuarli joshuarli requested review from a team as code owners April 30, 2026 22:04
Copy link
Copy Markdown
Member

@ceorourke ceorourke left a comment

Choose a reason for hiding this comment

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

ty josh

Copy link
Copy Markdown
Contributor

@saponifi3d saponifi3d left a comment

Choose a reason for hiding this comment

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

🙏 thanks for fixing!

# Explicit imports so selective testing can detect when these handlers change.
# They register via @action_handler_registry.register at import time (startup
# side-effect), so without these the static scanner has no edge from handler
# files to this test file.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, nice! thanks!

Comment thread src/sentry/integrations/github/handlers/github_handler.py
- test_action_type_in_migration: add `repo` to GitHub/GithubEnterprise
  test cases — the schema now requires repo in additional_fields
- test_rule.py: fix import order (ruff)
@joshuarli joshuarli added the Do Not Merge Don't merge label Apr 30, 2026
@joshuarli joshuarli requested a review from a team as a code owner April 30, 2026 22:48
@joshuarli joshuarli force-pushed the joshuarli/fix/reland-github-ticket-action-validation branch from 9692aa1 to b239c2b Compare April 30, 2026 23:00
@joshuarli joshuarli added Trigger: Override Selective Testing Run the full test suite; necessary in cases where selected tests are flaky due to reshuffling and removed Do Not Merge Don't merge labels Apr 30, 2026
@ceorourke ceorourke merged commit b653460 into master May 1, 2026
86 of 96 checks passed
@ceorourke ceorourke deleted the joshuarli/fix/reland-github-ticket-action-validation branch May 1, 2026 16:29
cleptric pushed a commit that referenced this pull request May 5, 2026
…ma and selective test fix (#114500)

## Summary

Re-applies #114095 (reverted in a630f3f due to a CI failure that
selective testing had missed)

**Original change (re-landed):**
- Adds a `data_schema` to `GithubActionHandler` requiring `repo` in
`additional_fields`, so the API rejects GitHub ticketing actions created
without a repo.
- Makes `GithubEnterpriseActionHandler` inherit from
`GithubActionHandler` to pick up the same validation.

**Schema fix:**
- Removes `integration` from `required` in `additional_fields`. The
`IssueAlertMigrator` translator moves `integration` into
`Action.integration_id` (a dedicated model field), so it never lands in
`additional_fields`. Requiring it in the schema caused `action.save()`
to raise a `ValidationError` when serializing existing
GitHub/GithubEnterprise rules via `WorkflowEngineRuleSerializer`.

---

edit: this doesn't really work so i'm prioritizing getting this PR back
in and overriding selective testing for now

**Selective testing fix:**
- Adds explicit `import sentry.integrations.github.handlers` and `import
sentry.integrations.github_enterprise.handlers` at the top of
`test_rule.py`. Both handlers self-register via
`@action_handler_registry.register` at import time (a Django startup
side-effect), which means the static import scanner had no edge from the
handler files to `test_rule.py`. The explicit imports make the
dependency traceable, so future handler changes will select this test.

---------

Co-authored-by: getsantry[bot] <66042841+getsantry[bot]@users.noreply.github.com>
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 Trigger: Override Selective Testing Run the full test suite; necessary in cases where selected tests are flaky due to reshuffling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants