Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

OPIK-796: Add new automation rule evaluator logs endpoint #1115

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

thiagohora
Copy link
Contributor

@thiagohora thiagohora requested a review from a team as a code owner January 22, 2025 15:29
@thiagohora thiagohora self-assigned this Jan 22, 2025
Copy link
Contributor

@BorisTkachenko BorisTkachenko left a comment

Choose a reason for hiding this comment

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

Overall looks good, yet I'm not much in the context. Also test is missing, as discussed on daily.
Approving to unblock, but I guess would be better also to get review from @andrescrz

Comment on lines +63 to +77
public static final String FIND_ALL = """

SELECT * FROM automation_rule_evaluator_logs

WHERE workspace_id = :workspace_id

<if(level)> AND level = :level <endif>

<if(ruleId)> AND rule_id = :rule_id <endif>

ORDER BY timestamp DESC

<if(limit)> LIMIT :limit <endif><if(offset)> OFFSET :offset <endif>

""";
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we write queries in each line, would be better to keep the same style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I will remove it next.

@thiagohora thiagohora merged commit 890b0d9 into main Jan 22, 2025
8 checks passed
@thiagohora thiagohora deleted the thiagohora/OPIK-796_only_endpoint_impl branch January 22, 2025 16:26
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants