feat(collection): added alias management - #159
Conversation
adityamparikh
left a comment
There was a problem hiding this comment.
Thanks @y-luis-rojo for the contribution. Seems like a useful capability to add.
Added inline comments as well as overall comments to address:
- Looks like the happy path in unit tests need actual assertions.
- Please add an AliasServiceIntegrationTest that uses Testcontainers to test adding, updating, removing aliases on a new or existing collection.
- Please update the docs with this new tool.
create-alias destructive and idempotent. Added tests. Updated documentation.
Thanks for the comments. I think all are now addressed. |
|
An end to end integration tests |
Sorry, missed to add it. Done. |
- Derive success from response.getStatus() instead of hardcoding true - Add @jsonformat to AliasResult timestamp for ISO 8601 consistency - Use separate TEST_COLLECTION_2 in integration test update scenario
4068819 to
72df1c5
Compare
|
@epugh Looks good to me. Can you please merge? |
thanks for the review! Still on vacation, but hoping to get some coding time this week. |
|
@y-luis-rojo looks like some lint checks failed... |
The Build job runs './gradlew classes testClasses spotlessCheck' as a fail-fast gate, and spotlessJavaCheck rejected the new alias files for indentation (spaces where the project's format expects tabs). Because Unit Tests and Integration Tests are gated behind Build, this single formatting miss blocked the entire pipeline. No functional change - output of './gradlew spotlessApply'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: adityamparikh <aditya.m.parikh@gmail.com>
createAlias_multipleAliasesCanExist (@order(5)) asserted that TEST_ALIAS still pointed at TEST_COLLECTION, but createAlias_updatesExistingAlias (@order(4)) had already repointed it to TEST_COLLECTION_2 - and asserts that it did. The expectation therefore contradicted the preceding test and failed deterministically: AliasServiceIntegrationTest > createAlias_multipleAliasesCanExist() FAILED java.lang.AssertionError at AliasServiceIntegrationTest.java:119 AliasService itself is correct; only the test expectation was stale. Rather than just correcting the expected value, this makes the test assert solely on aliases it creates itself, so it no longer depends on what an earlier test left TEST_ALIAS pointing at. TEST_ALIAS_2 is still left in place for the deletion tests that follow; TEST_ALIAS_3 is this test's own fixture and is cleaned up in the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: adityamparikh <aditya.m.parikh@gmail.com>
fix(alias): resolve CI failures on apache#159 (spotless + stale test expectation)
|
I approved. @epugh Could we merge? |
feat(collection): add alias management tools (list, create, delete)