Skip to content

add sqlalchemy backend #1

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

Closed
wants to merge 22 commits into from
Closed

add sqlalchemy backend #1

wants to merge 22 commits into from

Conversation

geospatial-jeff
Copy link
Collaborator

No description provided.

@geospatial-jeff
Copy link
Collaborator Author

geospatial-jeff commented Aug 10, 2022

This is mostly done. I still have to:

  • A few more passes through the entire codebase to make sure nothing is broken. All tests are passing and local dev works too but want to be extra careful here.
  • Switch from stac-fastapi monorepo packages to the singular packaged started in Remove backend packages stac-fastapi#450
  • Confirm that docs are deployed properly to github pages, and that the docs themselves look good.
  • Update README.

Copy link
Contributor

@duckontheweb duckontheweb left a comment

Choose a reason for hiding this comment

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

Is the intention to remove .github/workflows/publish.yml permanently and just deploy manually, or are you planning on replacing that with a different workflow?

Co-authored-by: Jon Duckworth <[email protected]>
@geospatial-jeff
Copy link
Collaborator Author

Is the intention to remove .github/workflows/publish.yml permanently and just deploy manually, or are you planning on replacing that with a different workflow?

Not permanently, I think deploy should be automated but just not sure what the deploy looks like yet. I think we should definitely deploy a docker container, not sure if we should also publish a library.

@geospatial-jeff
Copy link
Collaborator Author

I'd like to get this merged before switching everything over to stac-utils/stac-fastapi#450

@geospatial-jeff geospatial-jeff changed the title [WIP] add sqlalchemy backend add sqlalchemy backend Aug 13, 2022
@duckontheweb
Copy link
Contributor

I think we should definitely deploy a docker container, not sure if we should also publish a library.

A Docker container would be great. I think publishing the library would be helpful, too. I've built a couple applications already where I import and inherit from stac-fastapi.pgstac classes and I suspect others are doing the same.

Copy link
Contributor

@duckontheweb duckontheweb left a comment

Choose a reason for hiding this comment

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

This looks good to me @geospatial-jeff, and I was able to run everything based on the instructions in the README.

I had a couple of inline comments, but no big blockers. I also liked your suggestion in stac-utils/stac-fastapi#450(comment) that we consolidate the Docker images into a single Dockerfile. Should we maybe do that here as well?

pip install -e stac_fastapi/sqlalchemy
# or
pip install -e stac_fastapi/pgstac
pip install stac-fastapi.sqlalchey
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pip install stac-fastapi.sqlalchey
pip install stac-fastapi.sqlalchemy

minor typo

- "python"
- "/app/scripts/ingest_joplin.py"
- "http://app-pgstac:8082"
bash -c "./scripts/wait-for-it.sh stac-fastapi-sqlalchemy:8081 && alembic upgrade head && python /app/scripts/ingest_joplin.py http://stac-fastapi-sqlalchemy:8081"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bash -c "./scripts/wait-for-it.sh stac-fastapi-sqlalchemy:8081 && alembic upgrade head && python /app/scripts/ingest_joplin.py http://stac-fastapi-sqlalchemy:8081"
bash -c "./scripts/wait-for-it.sh -t 30 stac-fastapi-sqlalchemy:8081 && alembic upgrade head && python /app/scripts/ingest_joplin.py http://stac-fastapi-sqlalchemy:8081"

This is not necessarily related to this PR, but this almost always times out for me when starting from a fresh slate. The service seems to become available after about 19 or 20 seconds, so increasing this timeout to 30s should cover us.

@gadomski
Copy link
Member

Closing as OBE, see stac-utils/stac-fastapi#517 for details.

@gadomski gadomski closed this Mar 24, 2023
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