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

Plugin system #28

Merged
merged 6 commits into from
Mar 22, 2024
Merged

Plugin system #28

merged 6 commits into from
Mar 22, 2024

Conversation

frsaba
Copy link

@frsaba frsaba commented Feb 5, 2024

Made Amarillo into a python package and created a plugin system based on https://packaging.python.org/guides/creating-and-discovering-plugins/. Plugins are discovered dynamically from python packages placed under the amarillo/plugins namespace. A plugin can define a setup function that receives the FastAPI app object as a parameter.

Copy link
Member

@hbruch hbruch left a comment

Choose a reason for hiding this comment

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

A few remarks

@@ -1,10 +1,13 @@
from typing import Dict
from pydantic import Field
from pydantic_settings import BaseSettings

from typing import Optional
Copy link
Member

Choose a reason for hiding this comment

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

could you just add this to the already existing import in line 1 (or inversely add Dict from line 1)?

Comment on lines 8 to 10
# TODO: define these as required if metrics plugin is installed
metrics_user: Optional[str] = Field(None, env = 'METRICS_USER')
metrics_password: Optional[str] = Field(None, env = 'METRICS_PASSWORD')
Copy link
Member

Choose a reason for hiding this comment

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

It feels a bit strange to have plugin configuration in the core settings. Shouldn't this be done by the plugins requiring additional config?

Copy link
Author

Choose a reason for hiding this comment

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

I agree, I'll try and find a way to have this configuration on the plugin side

Comment on lines +9 to +18
"fastapi[all]==0.109.0",
"geopandas==0.14",
"uvicorn[standard]==0.23.2",
"pydantic[dotenv]==2.4.2",
"protobuf==3.20.3",
"starlette~=0.35",
"requests==2.31.0",
"pyproj==3.6.1",
"geojson-pydantic==1.0.1",
"watchdog==3.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

I guess version pinning in requirements.txt and requirements-dev.txt should be sufficient, or why do we need to duplicate it here?

Copy link
Author

Choose a reason for hiding this comment

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

requirements.txt does not get included in the python package, it is not considered when the package is pip installed.

@frankgerhardt
Copy link
Member

Rebase on new plugins branch

@frsaba frsaba changed the base branch from main to plugins March 22, 2024 15:08
@hbruch hbruch merged commit 7a5a89f into mfdz:plugins Mar 22, 2024
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