Skip to content

Conversation

@jjfrench
Copy link
Contributor

Porting over changes from NASA-IMPACT/veda-stac-ingestor#29

@sharkinsspatial
Copy link
Member

sharkinsspatial commented Jan 24, 2023

@jjfrench I'll try to review in detail tomorrow. The first thing I noted is that we have no additional tests for the logic in this PR. A bit deeper look revealed that our CI actions are not running internal lib tests at all. @alukach Did you have a specific approach you'd like to see for running internal lib tests such as https://github.com/developmentseed/cdk-pgstac/blob/main/lib/ingestor-api/runtime/tests/test_registration.py for new PRs? I've never built a generic construct like this so I'm unsure what the best practice should be.

@jjfrench I'll try to include the test runner in a separate PR and you may need to make an additional commit to trigger the automated action (preferably a commit containing a test for the new collection endpoint 😸 )

@sharkinsspatial
Copy link
Member

@jjfrench Here is the branch / PR with a test runner. You should be able to merge this branch and then build tests for your collection endpoint.

@jjfrench
Copy link
Contributor Author

@jjfrench Here is the branch / PR with a test runner. You should be able to merge this branch and then build tests for your collection endpoint.

Hey @sharkinsspatial, did you mean to link #10? Or was there another branch you were referencing?

@sharkinsspatial
Copy link
Member

🤦 That's the one.

@sharkinsspatial
Copy link
Member

sharkinsspatial commented Mar 3, 2023

@jjfrench I'm confused, it looks like that rather than fetching and merging my ci_tests branch that you copied code over and created your own commits 🤔. At this point given the number of extra chore commits you made it might best to

  1. Close this PR.
  2. Wait until Add python linting and test runner and corresponding Github action. #17 is approved and merged so that we have an available test runner.
  3. Create a new branch with just 2 commits. One containing the Collections API logic and another the unit tests.
  4. Submit a new PR from this branch.

@jjfrench jjfrench closed this Mar 3, 2023
@sharkinsspatial sharkinsspatial deleted the collection-endpoint branch March 16, 2023 19:18
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