Skip to content

Submit STAC using transactions API #297

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

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open

Conversation

ividito
Copy link
Contributor

@ividito ividito commented Feb 13, 2025

Proposed implementation for #288

Original ingest API support is maintained through an Airflow variable. I learned while setting this up that this is not a recommended approach, but it is consistent with what we do elsewhere.

@ividito ividito force-pushed the feat/transactions-api branch from ad3fac7 to cccda9e Compare February 13, 2025 17:15
Copy link
Contributor

@amarouane-ABDELHAK amarouane-ABDELHAK left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. I just left some small comments

- corrected type hint
- corrected exception in auth method
- removed unused cognito var
@smohiudd smohiudd mentioned this pull request Apr 16, 2025
4 tasks
@smohiudd
Copy link
Contributor

This branch has been deployed to SIT, but testing is currently blocked because the dev STAC API has switched over to using veda-keycloak and we still need to sort out how programatic auth will work.

@smohiudd
Copy link
Contributor

This branch was tested by switching on and off the TF_VAR_transactions_endpoint_enabled feature flag. I ingested the ida-ndwi-difference-test-transactions collection into the dev catalog.

@@ -104,7 +104,9 @@ module "sma-base" {
ASSUME_ROLE_WRITE_ARN = var.assume_role_write_arn,
SM2A_BASE_URL = module.sma-base.airflow_url,
CLOUDFRONT_TO_INVALIDATE = var.cloudfront_to_invalidate
CLOUDFRONT_PATH_TO_INVALIDATE = var.cloudfront_path_to_invalidate
CLOUDFRONT_PATH_TO_INVALIDATE = var.cloudfront_path_to_invalidate,
TRANSACTIONS_ENDPOINT_ENABLED = var.transactions_endpoint_enabled==true ? "True" : null
Copy link
Contributor

@smohiudd smohiudd Apr 23, 2025

Choose a reason for hiding this comment

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

I don't think tf bools are serialized in airflow in python. I used this hack but open if there's a better way.


group_kwgs = {"group_id": "Process", "tooltip": "Process"}

airflow_vars = Variable.get("aws_dags_variables")
Copy link
Member

Choose a reason for hiding this comment

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

If we set deserialize_json=True, do you know if we still need to do:

var.transactions_endpoint_enabled==true ? "True" : null

in main.tf?

Suggested change
airflow_vars = Variable.get("aws_dags_variables")
airflow_vars = Variable.get("aws_dags_variables", deserialize_json=True)

And maybe we refactor this file a bit so we call Variable.get in tasks rather than in the top level, since the docs seem to imply this is best practice: https://airflow.apache.org/docs/apache-airflow/2.8.4/best-practices.html#airflow-variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would love a refactor for this (we call the same set of Variable.get("aws_dags_variables") lines 16 times in our DAGs), but I think that's a different issue. .airflowignore keeps the DAG processor from parsing the groups and utils folders, so there's no performance loss by having these calls here.

CLOUDFRONT_PATH_TO_INVALIDATE = var.cloudfront_path_to_invalidate
CLOUDFRONT_TO_INVALIDATE = var.cloudfront_to_invalidate,
CLOUDFRONT_PATH_TO_INVALIDATE = var.cloudfront_path_to_invalidate,
TRANSACTIONS_ENDPOINT_ENABLED = var.transactions_endpoint_enabled==true ? "True" : null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related fix that removes the need for the ternary here: NASA-IMPACT/self-managed-apache-airflow#18

cc/ @amarouane-ABDELHAK

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