Skip to content

fix(ACI): Require 'repo' to be passed when creating GitHub ticket action#114095

Merged
ceorourke merged 3 commits intomasterfrom
ceorourke/ISWF-2265
Apr 30, 2026
Merged

fix(ACI): Require 'repo' to be passed when creating GitHub ticket action#114095
ceorourke merged 3 commits intomasterfrom
ceorourke/ISWF-2265

Conversation

@ceorourke
Copy link
Copy Markdown
Member

We have some front end validation here but if using the API directly it's possible to create a GitHub ticketing workflow that can't function if you fail to pass in the repo name. This PR adds validation on the GitHub ticketing action to make sure it's passed.

@ceorourke ceorourke requested review from a team as code owners April 27, 2026 21:57
@linear-code
Copy link
Copy Markdown

linear-code Bot commented Apr 27, 2026

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 27, 2026
Comment on lines +128 to +129
if not self.validated_data.get("data", {}).get("additional_fields", {}).get("repo"):
raise ValidationError("'repo' is required")
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.

since this is on the data field, could we just use the data_schema to enforce it on handler to enforce the json schema?

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.

lgtm!

@ceorourke ceorourke merged commit 36ce446 into master Apr 30, 2026
56 checks passed
@ceorourke ceorourke deleted the ceorourke/ISWF-2265 branch April 30, 2026 20:52
@ceorourke ceorourke added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Apr 30, 2026
@getsentry-bot
Copy link
Copy Markdown
Contributor

PR reverted: a630f3f

getsentry-bot added a commit that referenced this pull request Apr 30, 2026
…cket action (#114095)"

This reverts commit 36ce446.

Co-authored-by: ceorourke <29959063+ceorourke@users.noreply.github.com>
ceorourke pushed a commit that referenced this pull request May 1, 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>
cleptric pushed a commit that referenced this pull request May 5, 2026
…ion (#114095)

We have some front end validation here but if using the API directly
it's possible to create a GitHub ticketing workflow that can't function
if you fail to pass in the repo name. This PR adds validation on the
GitHub ticketing action to make sure it's passed.
cleptric pushed a commit that referenced this pull request May 5, 2026
…cket action (#114095)"

This reverts commit 36ce446.

Co-authored-by: ceorourke <29959063+ceorourke@users.noreply.github.com>
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: Revert Add to a merged PR to revert it (skips CI)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants