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

Alerting: Improve query types usage to prevent ts-errors #65238

Merged
merged 3 commits into from
Mar 29, 2023

Conversation

VikaCep
Copy link
Contributor

@VikaCep VikaCep commented Mar 22, 2023

What is this feature?

Removes the usage of ts-ignore when accessing the expr property for recording rules queries after #63260. Instead of that, proper types are assigned where needed.

Why do we need this feature?

To improve code quality.

@VikaCep VikaCep added area/alerting Grafana Alerting area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Mar 22, 2023
@VikaCep VikaCep added this to the 9.5.0 milestone Mar 22, 2023
@VikaCep VikaCep requested a review from a team as a code owner March 22, 2023 22:00
@VikaCep VikaCep self-assigned this Mar 22, 2023
@github-advanced-security
Copy link

You have successfully added a new CodeQL configuration .github/workflows/pr-codeql-analysis-javascript.yml:analyze. As part of the setup process, we have scanned this repository and found 29 existing alerts. Please check the repository Security tab to see all alerts.

Copy link
Member

@soniaAguilarPeiron soniaAguilarPeiron left a comment

Choose a reason for hiding this comment

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

LGTM! Maybe we could do it differently so we avoid adding a parameter to onChangeQuery?

Copy link
Member

@gillesdemey gillesdemey left a comment

Choose a reason for hiding this comment

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

This makes more sense to me 👍

On a side note I think we should probably have stricter typing than the generic AlertQuery (and it looks like mapToQuery doesn't event return a specific type, but just DataQuery & { expr: string } :(

@VikaCep
Copy link
Contributor Author

VikaCep commented Mar 28, 2023

I changed the approach by using a new helper function isPromOrLokiQuery which basically checks that the query's model contains the expr property (which is the only one that's not optional in PromQuery and LokiQuery). Using this method I no longer need mapToQuery or passing an extra parameter to onChangeQuery. Let me know if this looks better, thanks! 🙏

@VikaCep VikaCep merged commit 2b59d1a into main Mar 29, 2023
6 checks passed
@VikaCep VikaCep deleted the alerting/63260-fix-types branch March 29, 2023 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Grafana Alerting area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants