Skip to content

Search Configuration API #1666

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

Aashutosh-cognite
Copy link
Contributor

Description

Stacked on #1651

This PR introduces to search configurations API client to utilise list and update APIs for configurations. This will be used by Configurations resources to load search config resources and validate it.

Changelog

  • Patch
  • Minor
  • Skip

Copy link

github-actions bot commented Jun 23, 2025

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
19259 15032 78% 60% 🟢

New Files

File Coverage Status
cognite_toolkit/_cdf_tk/client/api/search_config.py 100% 🟢
TOTAL 100% 🟢

Modified Files

File Coverage Status
cognite_toolkit/_cdf_tk/client/_toolkit_client.py 85% 🟢
TOTAL 85% 🟢

updated for commit: a9b006d by action🐍

Copy link
Collaborator

@doctrino doctrino left a comment

Choose a reason for hiding this comment

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

A few adjustments. I think we should also ask if there is delete endpoint, as supporting this in Toolkit will be difficult without delete.

Comment on lines 21 to 39
@overload
def __call__(self) -> Iterator[SearchConfig]: ...

@overload
def __call__(self, chunk_size: int) -> Iterator[SearchConfigList]: ...

def __call__(self, chunk_size: int | None = None) -> Iterator[SearchConfig] | Iterator[SearchConfigList]:
"""Iterate over configurations.
Args:
chunk_size: The number of configurations to return in each chunk. None will return all configurations.

Yields:
SearchConfig or SearchConfigList

"""
return iter(self.list())

def __iter__(self) -> Iterator[SearchConfig]:
return self.__call__()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This endpoint does not support pagination, so lets skip this

Suggested change
@overload
def __call__(self) -> Iterator[SearchConfig]: ...
@overload
def __call__(self, chunk_size: int) -> Iterator[SearchConfigList]: ...
def __call__(self, chunk_size: int | None = None) -> Iterator[SearchConfig] | Iterator[SearchConfigList]:
"""Iterate over configurations.
Args:
chunk_size: The number of configurations to return in each chunk. None will return all configurations.
Yields:
SearchConfig or SearchConfigList
"""
return iter(self.list())
def __iter__(self) -> Iterator[SearchConfig]:
return self.__call__()

def __iter__(self) -> Iterator[SearchConfig]:
return self.__call__()

def update(self, configuration_update: SearchConfigWrite) -> SearchConfig:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def update(self, configuration_update: SearchConfigWrite) -> SearchConfig:
def upsert(self, configuration_update: SearchConfigWrite) -> SearchConfig:



@pytest.fixture(scope="session")
def existing_search_config(toolkit_client: ToolkitClient) -> dict:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not return a SearchConfig here?

Suggested change
def existing_search_config(toolkit_client: ToolkitClient) -> dict:
def existing_search_config(toolkit_client: ToolkitClient) -> SearchConfig:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also wanted count of already present configs thus was returning in dict. Could use tuple instead. with first element as count and second as SearchConfig. id can be fetched using SearchConfig.id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since not explicitly testing list() method so will just return SearchConfig

Comment on lines 45 to 49
def test_list_search_configurations(self, toolkit_client: ToolkitClient, existing_search_config: dict) -> None:
"""Test that we can list search configurations."""
search_configs = toolkit_client.search_configurations.list()
assert isinstance(search_configs, SearchConfigList)
assert len(search_configs) == existing_search_config["total_configs"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is already tested in the fixture

doctrino
doctrino previously approved these changes Jun 24, 2025
@doctrino doctrino added the waiting-for-risk-review Waiting for a member of the risk review team to take an action label Jun 24, 2025
Base automatically changed from search-admin-resource-type to main June 27, 2025 13:01
@Aashutosh-cognite Aashutosh-cognite dismissed doctrino’s stale review June 27, 2025 13:01

The base branch was changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-risk-review Waiting for a member of the risk review team to take an action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants