-
Notifications
You must be signed in to change notification settings - Fork 24
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
Draft: Use eoapi-cdk #135
Draft: Use eoapi-cdk #135
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handled in config.yaml
. Apart for some runtime and memory parameters that we still need to make configurable in eoapi-cdk.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on eoapi-template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here I am just calling the eoapi-cdk constructs, it's exactly like eoapi-template except for the runtimes
), | ||
allocated_storage=app_config.db_allocated_storage, | ||
instance_type=aws_ec2.InstanceType(app_config.db_instance_type), | ||
api_code={'entry': f'{APIS_RUNTIME_DIR}/db', 'index': 'handler.py', 'handler': 'handler'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this option doesn't actually exist yet, we need to add it...developmentseed/eoapi-cdk#72
"NAME": app_config.build_service_name("STAC API"), | ||
"description": f"{app_config.stage} STAC API", | ||
}, | ||
api_code={'entry': f'{APIS_RUNTIME_DIR}/stac', 'index': 'handler.py', 'handler': 'handler'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to show the idea, but it isn't going to work as is because we're relying on Dockerfiles in infrastructure/aws/Dockerfiles
and the runtimes are at the root of the repo.
Can we remove the Dockerfiles and have an accessible runtime folder with just a handler.py and requirements.txt @vincentsarago ?
If in the future we need to pass Docker commands we can allow for BundlingOptions
, like we had discussed in developmentseed/eoapi-cdk#66 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the other APIs below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the Dockerfiles and have an accessible runtime folder with just a handler.py and requirements.txt @vincentsarago ?
sadly no! some of the packages will be to be, which is why we need the custom dockerfile. And we also have custom code which can't really fit in an handler.py
if app_config.auth_provider_jwks_url: | ||
stac_ingestor_env["JWKS_URL"] = app_config.auth_provider_jwks_url | ||
|
||
stac_ingestor = StacIngestor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want this @vincentsarago ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 🙏
public_db_subnet: True # DB in public subnet | ||
bastion_host: False # no bastion host | ||
acm_certificate_arn: some_arn # for custom domain names | ||
stac_api_custom_domain: stac.eoapi.dev # domain name for STAC API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is going to create the custom domains
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I don't think we want to create the domain, because we could deploy this repo to multiple stage so we don't want to have conflits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove that entry @vincentsarago, but what kind of conflict are you talking about ? If we want to deploy to another stage we can just change the domain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly that if you forget to update it by default then you are overriding the production endpoint, maybe we can just have this commented out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see what you mean. We could have a config file per stage, that's sort of what we're doing in MAAP.
But I'll turn off the domain for now since it wasn't in your code and just stick to converting what's existing to use the constructs.
Can always turn on the option later.
closing this in favor of #144. |
Partially closes #94
@vincentsarago this is a preliminary draft, not ready but I wanted to show the idea before doing more work. What do you think ?
A first basic question : is this all necessary or we can just get rid of
infrastructure/aws
and deploy this https://github.com/developmentseed/eoapi-template with the custom runtimes ? Would be better, I guess?