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

feat: remove cloudfront distribution and custom alternate domain from backend #422

Merged
merged 11 commits into from
Aug 23, 2024

Conversation

botanical
Copy link
Member

@botanical botanical commented Aug 15, 2024

Issue

#403
#221

What + Why

  • Remove cloudfront distribution CDK from veda-backend because it's now managed in veda-routes
  • Update values passed into ingestor config to derive endpoint values using the custom host and stac and raster API root paths
  • Remove configuration and support for hosting duplicate stac and raster apis as alternative domains.

Testing?

Deployed to UAH

@botanical botanical changed the title feat: remove cloudfront distribution from backend feat: remove cloudfront distribution and custom alternate domain from backend Aug 16, 2024
Copy link
Contributor

@ciaransweet ciaransweet left a comment

Choose a reason for hiding this comment

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

Just one open question but LGTM!

config.py Show resolved Hide resolved
@botanical
Copy link
Member Author

Going to hold off on merging this until after next release

app.py Outdated
@@ -120,23 +104,27 @@ def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
db_secret_name = database.pgstac.secret.secret_name
db_security_group = database.db_security_group

base_api_url = f"https://{veda_app_settings.stage_name()}.{veda_app_settings.veda_custom_host}".strip(
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't VEDA_CUSTOM_HOST already include the stage name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah it does!

Copy link
Member Author

Choose a reason for hiding this comment

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

app.py Outdated
# ingestor config requires references to other resources, but can be shared between ingest api and bulk ingestor
ingestor_config = ingest_config(
stage=veda_app_settings.stage_name(),
stac_db_security_group_id=db_security_group.security_group_id,
stac_api_url=stac_api.stac_api.url,
raster_api_url=raster_api.raster_api.url,
stac_api_url=stac_api_url,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we could handle inferring the cloudfront urls in config with a property or function

@property
def veda_stac_api_cf_url(self) -> str:
    """inferred cloudfront url of the stac api if app is configured with a custom host and root path"""
    if self.veda_custom_host and self.veda_stac_root_path:
        return f"https://{self.veda_custom_host}{self.veda_stac_root_path}"
    return None 

and then conditionally use them here so that we have a way to use the API gateway url if a cloudfront is not used.

stac_api_url=settings.stac_api_cf_url if settings.stac_api_cf_url else stac_api.stac_api.url

@botanical botanical requested review from smohiudd and anayeaye August 22, 2024 22:10
Copy link
Collaborator

@anayeaye anayeaye left a comment

Choose a reason for hiding this comment

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

Sorry I didn't hit send on these yesterday. The only change I think we might need is the suggestion about handling both api gateway urls and cloudfront urls #422 (comment)

veda_routes.create_route_records(stage=veda_app_settings.stage_name())


# TODO this conditional supports deploying a second set of APIs to a separate custom domain and should be removed if no longer necessary
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉

ingest_api/infrastructure/config.py Show resolved Hide resolved
ingest_api/infrastructure/construct.py Show resolved Hide resolved
@anayeaye anayeaye mentioned this pull request Aug 23, 2024
1 task
@botanical botanical requested a review from anayeaye August 23, 2024 20:49
@botanical botanical merged commit d6aedd7 into develop Aug 23, 2024
4 checks passed
@botanical botanical deleted the jt/remove-cloudfront branch August 23, 2024 21:25
anayeaye added a commit that referenced this pull request Oct 15, 2024
…lidation, add nlcd colormap (#434)

**Breaking**
feat(routes)!: remove cloudfront distribution and custom alternate
domain from backend #422

**Added** 
feat(raster-api): added custom NLCD colormap
#433
feat: add ecr endpoint
#432

**Fixed**
fix(tests): #425
fix(ingest): improved datetime validations
#419

**Changed**
ci: remove automated deployments to staging
#438
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.

4 participants