Skip to content

Merge query_rule and query_ruleset namespaces into query_rules #2656

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

Merged
merged 10 commits into from
Jun 28, 2024

Conversation

kderusso
Copy link
Member

Moves the query_rule and query_ruleset namespaces into a single top level query_rules namespace.

Followup to #2639

Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
query_rules.query_rule_delete Missing test Missing test
query_rules.query_rule_get Missing test Missing test
query_rules.query_rule_put Missing test Missing test
query_rules.query_ruleset_delete Missing test Missing test
query_rules.query_ruleset_get Missing test Missing test
query_rules.query_ruleset_list Missing test Missing test
query_rules.query_ruleset_put Missing test Missing test

You can validate these APIs yourself by using the make validate target.

Copy link
Member

@pquentin pquentin 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 this! I believe having a single namespace is much better! I'm only wondering about the endpoint names defined in the JSON spec, as they are used in client calls. With this PR, taking get as an example, here's how users will call those APIs:

Client call API call
client.query_rules.query_ruleset_get(...) GET /query_rules/{ruleset_id}
client.query_rules.query_rule_get(...) GET /query_rules/{ruleset_id}/_rule/{rule_id}

The two issues I'm seeing are:

  1. Other APIs put the verb first, eg. indices.put_mapping or inference.put_model
  2. There's a repetition around query_rule

What are your thoughts around using those names instead?

  • client.query_rules.get_ruleset(...)
  • client.query_rules.get_rule(...)

@pquentin pquentin changed the title Kderusso/move query rules namespace Merge query_rule and query_ruleset namespaces into query_rules Jun 27, 2024
@kderusso
Copy link
Member Author

What are your thoughts around using those names instead?

client.query_rules.get_ruleset(...)
client.query_rules.get_rule(...)

😅 Yeah, those are cleaner, I'll update both PRs today.

@kderusso kderusso requested a review from pquentin June 27, 2024 18:39
@kderusso kderusso marked this pull request as ready for review June 27, 2024 19:58
Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
query_rules.delete_rule Missing test Missing test
query_rules.delete_ruleset Missing test Missing test
query_rules.get_rule Missing test Missing test
query_rules.get_ruleset Missing test Missing test
query_rules.list_rulesets Missing test Missing test
query_rules.put_rule Missing test Missing test
query_rules.put_ruleset Missing test Missing test

You can validate these APIs yourself by using the make validate target.

Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
query_rules.delete_rule Missing test Missing test
query_rules.delete_ruleset Missing test Missing test
query_rules.get_rule Missing test Missing test
query_rules.get_ruleset Missing test Missing test
query_rules.list_rulesets Missing test Missing test
query_rules.put_rule Missing test Missing test
query_rules.put_ruleset Missing test Missing test

You can validate these APIs yourself by using the make validate target.

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM. I had to run git merge locally as GitHub reported conflicts. (The actual git merge did not have any conflicts.) And I ran make contrib to fix the OpenAPI specs.

@pquentin pquentin merged commit cda6fb5 into main Jun 28, 2024
6 checks passed
@pquentin pquentin deleted the kderusso/move-query-rules-namespace branch June 28, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants