Skip to content

fix: amazonia_1 #44

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

fix: amazonia_1 #44

wants to merge 7 commits into from

Conversation

@jjfrench
Copy link
Contributor

@jjfrench jjfrench commented Jul 14, 2024

Description

Slightly larger PR than planned but required a couple of fixes to get it working. Based off changes in aws-asdi-pgstac

FIXED

CHANGED

  • updated lambdas to use correct secret_arn values based on their regions
  • bumped aws-cdk/aws-cdk-lib versions for lambda default node18 runtime (node14 no longer supported)
  • updated to use model_validator (root_validator deprecated)

@wildintellect wildintellect requested a review from hrodmn July 15, 2024 15:50
hrodmn
hrodmn reviewed Jul 15, 2024
View reviewed changes
Copy link
Contributor

@hrodmn hrodmn left a comment

Choose a reason for hiding this comment

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

I think these changes look good! Do you know if we have asdi workflow secrets for all AWS regions? And does the region of the secret_arn have any impact on downstream operations?

sharkinsspatial
sharkinsspatial requested changes Jul 19, 2024
View reviewed changes
Copy link
Member

@sharkinsspatial sharkinsspatial left a comment

Choose a reason for hiding this comment

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

@jjfrench Can you separate out your changes to the core stactools-pipelines code (setup.py, pipeline.py, tox.ini) into a separate PR for clarity. We can merge that to main first and then merge your amazonia 1 specific changes.

sharkinsspatial
sharkinsspatial reviewed Jul 19, 2024
View reviewed changes
stactools_pipelines/pipelines/aws_noaa_oisst_avhrr_only/config.yaml
Copy link
Member

@sharkinsspatial sharkinsspatial Jul 19, 2024

Choose a reason for hiding this comment

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

@jjfrench Let's just leave the other pipelines alone for this PR and we can go back and update them if need be in another one.

This was referenced Jul 19, 2024
Closed
Open
@jjfrench
Copy link
Contributor Author

jjfrench commented Jul 19, 2024

Cherry-picked commits from this branch so that we wouldn't have commits adding and removing the changes to the other pipelines. Closing.

@jjfrench jjfrench closed this Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sharkinsspatial sharkinsspatial sharkinsspatial requested changes

@hrodmn hrodmn hrodmn left review comments

@wildintellect wildintellect Awaiting requested review from wildintellect

Assignees

@jjfrench jjfrench

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jjfrench @sharkinsspatial @hrodmn