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

Issue 10: Move state machine to terraform module #11

Merged
merged 31 commits into from
Mar 25, 2024

Conversation

voxparcxls
Copy link
Contributor

Github Issue: #10

Description

Move bignbit state machine definition to terraform module

Overview of work done

TF update to step.tf, iam_step.tf, main.tf

Overview of integration done

CI Build, Workflow

PR checklist:

  • Linted
  • Updated unit tests
  • Updated changelog
  • Integration testing

See Pull Request Review Checklist for pointers on reviewing this pull request

Copy link
Member

@frankinspace frankinspace left a comment

Choose a reason for hiding this comment

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

Can you also update the examples/cumulus-tf to show how the module should be used in a cumulus deployment now?

@frankinspace frankinspace linked an issue Mar 7, 2024 that may be closed by this pull request
@voxparcxls
Copy link
Contributor Author

@frankinspace
Because it is now in /terraform , shouldn't this be named {}-BrowseImageWorkflow instead of {}-sm:

name = "${local.ec2_resources_name}-sm"

@frankinspace
Copy link
Member

@voxparcxls That's why I was wondering if just putting the state machine definition into the workflow module will work or not. Doesn't https://github.com/nasa/cumulus/releases/download/v16.1.2/terraform-aws-cumulus-workflow.zip create a state machine resource based on the definition passed in to it? I think we might end up with the same state machine defined twice

@frankinspace
Copy link
Member

Last time we talked about this we had talked about the approach of calling the state machine inside the module from the cumulus workflow state machine that needs to use the module.

@voxparcxls
Copy link
Contributor Author

voxparcxls commented Mar 8, 2024

Yes, terraform-aws-cumulus-workflow.zip has the VARs that inherit the definition. I think you're right.

Now, there is resource state_machine in modules/big_and_bit_module and browse_image_workflow

So it ends up creating both:

  • svc-haile-cumulus-bignbit-BrowseImageWorkflow (from big_and_bit_module)
  • haile-cumulus-BrowseImageWorkflow (from browse_image_workflow)

Perhaps finding a different way to declare the JSON definition and then have it invoke in cumulus workflow is one way to go about it.

@frankinspace

@voxparcxls
Copy link
Contributor Author

voxparcxls commented Mar 15, 2024

Correct, But I believe the created module ‘bignbit_module’ should provide that file once it’s there.

With this module path:

in directory ../../terraform

definition = templatefile("../../terraform/state_machine_definition.json", {

@voxparcxls
Copy link
Contributor Author

Correct, But I believe the created module ‘bignbit_module’ should provide that file once it’s there.

With this module path:

in directory ../../terraform

definition = templatefile("../../terraform/state_machine_definition.json", {

@frankinspace

The Nested workflow method doesn’t cover the problem of duplicate state machines once that “resource” is activate by the cumulus deployment big_and_bit_module and later used as a definition in browse_image_workflow module.



With the single JSON file state_machine_definition.json , its accessible for cumulus deploy and bignbit repo.


Update (bignbit repository):

  • With the step.tf staying in examples/cumulus-tf/
    • step.tf resource sfn_state_machine and the browse_image_workflow are both able to located the state_machine_definition from ../../terraform/state_machine_definition.json

BLOCKER: There is a block for browse_image_workflow deploy in our bignbit CICD. 

There are variables and outputs required in that declared in the large cumulus https://github.com/nasa/cumulus/releases/download/v16.1.2/terraform-aws-cumulus.zip

I think this can be its own ticket. This PR can deal with the combined state_machine

@voxparcxls voxparcxls marked this pull request as ready for review March 18, 2024 17:53
voxparcxls and others added 2 commits March 21, 2024 14:41
* Try outputing rendered template

* Update manual build handling

* fix typo in environment

* permission magic?

* cleanup
@frankinspace
Copy link
Member

@voxparcxls I added a commit with the suggestion we discussed on Wed. Please review and I think we're good to merge

@frankinspace frankinspace merged commit b5d8e32 into develop Mar 25, 2024
4 checks passed
@frankinspace frankinspace deleted the issue/bignbit-statemachine branch March 25, 2024 15:40
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.

Move combined big and pobit state machine into terraform module
2 participants