-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(triage signals): Fixability based stopping point for autofix #103076
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
Conversation
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
| event_id: str, | ||
| user_id: int | None, | ||
| auto_run_source: str, | ||
| stopping_point: AutofixStoppingPoint | None = None, |
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.
we're changing the signature of the task which could cause issues as old/new versions exist at the same time, but since we're adding a default to the parameter, this is okay. (we may want to add **kwargs to the signature for future in case a parameter gets removed)
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.
Yeah I thought a default should be okay. I think I will keep as is for now because this will become a required parameter once the the fallback stuff is removed.
JoshFerge
left a comment
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.
how does this change interact with what values people have configured?
Currently it does not. My next PR will have the UI change the completely removes that settings and turns automation into a on and off toggle. |
| return | ||
|
|
||
| stopping_point = None | ||
| if features.has("organizations:triage-signals-v0", group.organization): |
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.
hmm does this flag need to be a ProjectFeature, not an OrganizationFeature? then pass group.project instead
looks like this is tested, but i'm not familiar w/ how it's mocked under the hood
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.
why does it need to be a project feature?
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.
Hmm it not sure either. This said organization here so I went with organizationFeature.
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.
also, is this feature registered? was that done in a separate PR?
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.
Or since this only applies to the Seer project should I change the feature in flagpole to be a project feature?
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.
the flag checks project_id
what do you mean by this?
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.
the feature flag's filter is on project_id
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.
oh... wait can an organization feature even filter on project_id?
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.
https://develop.sentry.dev/backend/application-domains/feature-flags/flagpole/
doesn't seem like it. @Mihir-Mavalankar can you double check these docs and make sure our flag usage is correct?
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.
Yeah that's right. I have changed it to a project feature now. Options PR is merged and manager is updated in this PR.
Issues attributed to commits in this pull requestThis pull request was merged and Sentry observed the following issues: |
PR Details