Skip to content
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

feature: Make the secondary index range key nullable #170

Conversation

ShauniArima
Copy link
Contributor

As said in #103, Tempest does not allow using an empty sort key as secondary index.

I do not know how to test it correctly. This part of the code seems hard to test. Does someone have some tips for me?

@ShauniArima ShauniArima force-pushed the feature/support-global-secondary-index-without-sort-key branch from 2c3042e to ddb19e8 Compare March 5, 2024 21:55
@kyeotic
Copy link
Collaborator

kyeotic commented Mar 6, 2024

This looks good, thanks for taking the time to put this together. I need to dig into this to make sure its not going to cause any other issues, but I think I should be able to accept it. I should have some time tomorrow or friday to do that.

@zhxnlai
Copy link
Collaborator

zhxnlai commented Mar 6, 2024

Thanks for the contribution. You may also want to update that code that integrates with AWS SDK V1: it currently checks for requireNotNull on global secondary index sort key. https://github.com/cashapp/tempest/blob/main/tempest/src/main/kotlin/app/cash/tempest/internal/V1.kt#L85

@ShauniArima
Copy link
Contributor Author

Thanks for the contribution. You may also want to update that code that integrates with AWS SDK V1: it currently checks for requireNotNull on global secondary index sort key. https://github.com/cashapp/tempest/blob/main/tempest/src/main/kotlin/app/cash/tempest/internal/V1.kt#L85

Thanks!
I have updated the code for AWS SDK V1 too 😄

@kyeotic kyeotic merged commit c13b682 into cashapp:main Mar 7, 2024
2 checks passed
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.

3 participants