Skip to content

feat(collection): added alias management#159

Open
y-luis-rojo wants to merge 4 commits into
apache:mainfrom
y-luis-rojo:feature/alias-management
Open

feat(collection): added alias management#159
y-luis-rojo wants to merge 4 commits into
apache:mainfrom
y-luis-rojo:feature/alias-management

Conversation

@y-luis-rojo

Copy link
Copy Markdown

feat(collection): add alias management tools (list, create, delete)

@adityamparikh adityamparikh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @y-luis-rojo for the contribution. Seems like a useful capability to add.
Added inline comments as well as overall comments to address:

  1. Looks like the happy path in unit tests need actual assertions.
  2. Please add an AliasServiceIntegrationTest that uses Testcontainers to test adding, updating, removing aliases on a new or existing collection.
  3. Please update the docs with this new tool.

Comment thread src/main/java/org/apache/solr/mcp/server/collection/AliasService.java Outdated
Comment thread src/main/java/org/apache/solr/mcp/server/collection/AliasService.java Outdated
create-alias destructive and idempotent.
Added tests.
Updated documentation.
@y-luis-rojo

Copy link
Copy Markdown
Author

Thanks @y-luis-rojo for the contribution. Seems like a useful capability to add. Added inline comments as well as overall comments to address:

  1. Looks like the happy path in unit tests need actual assertions.
  2. Please add an AliasServiceIntegrationTest that uses Testcontainers to test adding, updating, removing aliases on a new or existing collection.
  3. Please update the docs with this new tool.

Thanks for the comments. I think all are now addressed.

@adityamparikh

Copy link
Copy Markdown
Contributor

An end to end integration tests AliasServiceIntegrationTest which uses testcontainers still needs to be added.

@y-luis-rojo

Copy link
Copy Markdown
Author

An end to end integration tests AliasServiceIntegrationTest which uses testcontainers still needs to be added.

Sorry, missed to add it. Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants