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

Add orca/orca_recovery_workflow modules, update docs #93

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Jkovarik
Copy link

This PR adds two daac user modules:

As this initial effort was in support of OBDAAC who are using the Cumulus default database module, the module is currently assuming use of the rds module. Improving this to allow for additional database clusters is beyond the scope of that work, but this module could be improved to accommodate other configurations.

  • orca_recovery_workflow -- This module takes outputs from the cumulus and orca modules to build a default 'recovery' workflow using the recovery adapter lambda Cumulus Core provides.

I'd be happy to discuss further, provide demonstrations of the deployed modules, etc, as needed. Thanks for your consideration.

@Jkovarik
Copy link
Author

Reviewers - please note this work is still in progress, as it's in review in the OBDAAC project. I'm posting here to start getting feedback as I believe it's in a form complete enough for additional feedback.

@mikedorfman
Copy link
Collaborator

@Jkovarik - thank you for this PR! Does this still reflect what OBDAAC is using (just checking to see whether there were any changes/updates after the OBDAAC review)? I'm planning on using much of this in NSIDC's use of ORCA.

@@ -0,0 +1,22 @@
terraform {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this second terraform block is redundant (since required_version is specified on line 5).

Copy link
Author

Choose a reason for hiding this comment

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

Removed: b056ef3

@Jkovarik
Copy link
Author

Jkovarik commented Nov 2, 2023

@mikedorfman thanks for the comment - I'm planning on reviewing where that finally landed in our merge PRs and this repo just to make sure things are up to date by the end of this sprint.

@Jkovarik
Copy link
Author

Jkovarik commented Nov 6, 2023

@mikedorfman I checked the current state of the OB branch with Orca enabled against this PR and things look good. Please let me know if you run into issues integrating this PR/need help/etc.

Copy link
Collaborator

@mikedorfman mikedorfman left a comment

Choose a reason for hiding this comment

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

This looks great from my perspective! NSIDC has deployed these changes and ORCA appears to be working as-expected.

## --------------------------

## TODO - Decide if it's valueable to allow for an alternate cluster to the rds module cluster
## OBDAAC implementation makes it YAGNI
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these TODOs still relevant? Also I am not familiar with YAGNI?

@@ -0,0 +1,2 @@
orca_db_user_password="<db password goes here>"
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel we should probably be promoting the use of ENV vars instead of tfvars using DB passwords stored in a repo. Maybe just a note?

Copy link
Author

Choose a reason for hiding this comment

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

In general agree - that's more a function of how Orca is offering configuration as part of it's module. I believe we opted to generate the secrets file via script pulling from a secret store in the bamboo build, for what it's worth.

@Jkovarik
Copy link
Author

Jkovarik commented Dec 6, 2023

Thanks for the review @mattp0! Merging will need to be done via a maintainer - I'm considering my involvement with the PR done at this point without further engagement from the team.


variable "elb_account_id" {
type = string
default = "797873946194"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think an account_id should have a default value. Am I missing something?

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