search

nasa / cumulus

Cumulus Framework + Cumulus API
Other
248 stars 103 forks source link

CUMULUS-3641 - Update Collections LIST endpoint to query Postgres basic #3681

Closed Nnaga1 closed 3 weeks ago

Nnaga1 commented 1 month ago

Summary: Summary of changes

Addresses CUMULUS-XX: Develop amazing new feature

Changes

PR Checklist

Nnaga1 commented 4 weeks ago

Please enable ts-check on the endpoints logic!

I don't agree with this, this creates errors in virtually every other function when it does getEsClient() (put, del, etc.) except for list pretty much (the fix for that is easy), I don't think I should add something that makes me change work that isn't mine or even related to mine and doesn't really have much value outside of a temporary fix/check

Jkovarik commented 4 weeks ago

Please enable ts-check on the endpoints logic!

I don't agree with this, this creates errors in virtually every other function when it does getEsClient() (put, del, etc.) except for list pretty much (the fix for that is easy), I don't think I should add something that makes me change work that isn't mine or even related to mine and doesn't really have much value outside of a temporary fix/check

Respectfully, it's required per our team ADRs, and something we should be doing in all of our other PRs. It's not expected that we fix all of the errors noted, but all code in the scope of the updates should be properly type annotated.

We should have a team conversation about this - if the team wants to change that stance, we should make that a deliberate decision.

Until this is resolved this cannot be merged.

Nnaga1 commented 4 weeks ago

Please enable ts-check on the endpoints logic!

I don't agree with this, this creates errors in virtually every other function when it does getEsClient() (put, del, etc.) except for list pretty much (the fix for that is easy), I don't think I should add something that makes me change work that isn't mine or even related to mine and doesn't really have much value outside of a temporary fix/check

Respectfully, it's required per our team ADRs, and something we should be doing in all of our other PRs. It's not expected that we fix all of the errors noted, but all code in the scope of the updates should be properly type annotated.

We should have a team conversation about this - if the team wants to change that stance, we should make that a deliberate decision.

Until this is resolved this cannot be merged.

I think an arch meeting topic would be good, I think there is still something I am misunderstanding as to the why/what that you can probably explain better through words 😆 , and it doesnt seem like itll be too magnitudal of a change anyways so. I just dont want to mess with the rest of the file (which does have ts errors, a good amount) if this one spot is going to be a temporary change/ doesnt even give issues/wasn't like that before.

Jkovarik commented 3 weeks ago

@Nnaga1 the changes in cea3c14 and db45ecd look good re: the collection endpoint typings. Thank you!