[Issue #2351, #2472] Migrate to platforms step function pattern #2506

coilysiren · 2024-10-17T00:13:15Z

Summary

Relates to: #2351, #2472

Time to review: 5 mins

Changes proposed

There are two goals to this PR:

Make a material change that gets us closer in line with the platform. Specifically we are pulling in Add scheduled jobs navapbc/template-infra#745
Updates the step functions in advance of adding another step function - for the terraform in ECS stuff

Context for reviewers

The majority of the diff is security related, specifically:

via guidance from @lorenyu, the permissions for managing the step function and workflow triggers were moved off of the task exec role. that's the diff on infra/modules/service/access-control.tf
those permissions were moved to infra/modules/service/scheduler_role.tf and infra/modules/service/workflow_orchestrator_role.tf

The rest of the diff is around easy of configuration:

infra/modules/service/scheduled_jobs.tf represents the new, highly configurable, way to create scheduled jobs
infra/api/app-config/env-config/scheduled_jobs.tf shows how two scheduled jobs are configured

Additional information

coilysiren · 2024-10-17T05:38:10Z

infra/api/app-config/env-config/scheduled_jobs.tf

+    copy-oracle-data = {
+      task_command        = ["poetry", "run", "flask", "data-migration", "copy-oracle-data"]
+      schedule_expression = "rate(2 minutes)"
+    }


@chouinar fancy new simple way to setup scheduled jobs

Thanks, this is simple enough that I can set things up / not bother you for any adjustments we make.

mdragon · 2024-10-17T14:49:08Z

infra/api/app-config/env-config/scheduled_jobs.tf

+  scheduled_jobs = {
+    copy-oracle-data = {
+      task_command        = ["poetry", "run", "flask", "data-migration", "copy-oracle-data"]
+      schedule_expression = "rate(2 minutes)"


Should we include enabled/disabled here too, or does a null schedule or something like that cause the job to just not run? Just thinking there may be times we want to keep the job from running, but not delete it from here entirely.

Heh, I believe I had this same question in the original PR.

I think it's a good idea, so I'm gonna add it

Added! 1ba3d06

infra/modules/service/scheduled_jobs.tf

mdragon

Approving as is. If we do want to facilitate pausing jobs via config as suggested in the comments, I'm happy to have that come in a future ticket.

@lorenyu

## Summary Relates to: #2351, #2472 ### Time to review: __5 mins__ ## Changes proposed There are two goals to this PR: 1. Make a material change that gets us closer in line with the platform. Specifically we are pulling in navapbc/template-infra#745 2. Updates the step functions in advance of adding another step function - for the terraform in ECS stuff ## Context for reviewers The majority of the diff is security related, specifically: - via guidance from @lorenyu, the permissions for managing the step function and workflow triggers were moved off of the task exec role. that's the diff on `infra/modules/service/access-control.tf` - those permissions were moved to `infra/modules/service/scheduler_role.tf` and `infra/modules/service/workflow_orchestrator_role.tf` The rest of the diff is around easy of configuration: - `infra/modules/service/scheduled_jobs.tf` represents the new, highly configurable, way to create scheduled jobs - `infra/api/app-config/env-config/scheduled_jobs.tf` shows how two scheduled jobs are configured ## Additional information <img width="1238" alt="image" src="https://github.com/user-attachments/assets/24e6e50e-74b0-4cf8-b61b-bad34af90118"> <img width="1237" alt="image" src="https://github.com/user-attachments/assets/aa297341-8d7d-4ac7-bcc9-ef102df25e05">

## Summary Relates to #2548 ### Time to review: __2 mins__ ## Changes proposed Swaps the analytics application to the new step functions pattern that I originally merged in #2506. The size of the red diff and the simplicity of the new configuration show have valuable the new pattern is. ## Context for reviewers The step function is now running, but the CLI itself is currently broken. eg. the CLI is currently broken everywhere, even locally. Fixing that is someone else's job though!

@coilysiren

## Summary Fixes #2665 ### Time to review: __1 min__ ## Changes proposed > What was added, updated, or removed in this PR. Added `gh-transform-and-load` command to existing `make gh-data-export` command. I'm not sure if this is sufficient or correct, but I'm taking a guess based on what I see in #2546 and #2506. ## Context for reviewers > Testing instructions, background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers. Explain how the changes were verified. In the analytics work stream, we have a new CLI command `make gh-transform-and-load` for transforming and loading (some) GitHub data. Per issue #2665, that command should be run daily, after the existing `gh-data-export` command which exports data from Github. I see that `scheduled_jobs.tf` seems to be the mechanism by which `make gh-data-export` runs daily. In this PR I'm taking and educated guess and attempting to add `gh-transform-and-load` to the existing job, and requesting feedback from @coilysiren as to whether this is the correct approach. ## Additional information > Screenshots, GIF demos, code examples or output to help show the changes working as expected. Co-authored-by: kai [they] <[email protected]>

@coilysiren

## Summary Fixes #2665 ### Time to review: __1 min__ ## Changes proposed > What was added, updated, or removed in this PR. Added `gh-transform-and-load` command to existing `make gh-data-export` command. I'm not sure if this is sufficient or correct, but I'm taking a guess based on what I see in #2546 and #2506. ## Context for reviewers > Testing instructions, background context, more in-depth details of the implementation, and anything else you'd like to call out or ask reviewers. Explain how the changes were verified. In the analytics work stream, we have a new CLI command `make gh-transform-and-load` for transforming and loading (some) GitHub data. Per issue #2665, that command should be run daily, after the existing `gh-data-export` command which exports data from Github. I see that `scheduled_jobs.tf` seems to be the mechanism by which `make gh-data-export` runs daily. In this PR I'm taking and educated guess and attempting to add `gh-transform-and-load` to the existing job, and requesting feedback from @coilysiren as to whether this is the correct approach. ## Additional information > Screenshots, GIF demos, code examples or output to help show the changes working as expected. Co-authored-by: kai [they] <[email protected]>

Migrate to platforms step function pattern

9e4e84b

github-actions bot added terraform infra labels Oct 17, 2024

coilysiren added 4 commits October 16, 2024 17:33

errant space

6b4038d

swap over load transform as well

217edf4

space

066475f

appease checkov

ef27b86

coilysiren marked this pull request as ready for review October 17, 2024 05:37

coilysiren requested review from acouch and mdragon as code owners October 17, 2024 05:37

Merge branch 'main' into kai/sfns

f563227

coilysiren requested a review from chouinar October 17, 2024 05:37

coilysiren commented Oct 17, 2024

View reviewed changes

mdragon reviewed Oct 17, 2024

View reviewed changes

infra/modules/service/scheduled_jobs.tf Outdated Show resolved Hide resolved

mdragon previously approved these changes Oct 17, 2024

View reviewed changes

add state

1ba3d06

coilysiren dismissed mdragon’s stale review via 1ba3d06 October 17, 2024 16:42

coilysiren requested a review from mdragon October 17, 2024 16:42

mdragon approved these changes Oct 17, 2024

View reviewed changes

coilysiren merged commit 1277e32 into main Oct 17, 2024
7 checks passed

coilysiren deleted the kai/sfns branch October 17, 2024 17:42

coilysiren mentioned this pull request Oct 23, 2024

[Issue #2548] Swap analytics to new step function pattern #2546

Merged

DavidDudas-Intuitial mentioned this pull request Nov 6, 2024

[Issue 2665] Add gh-transform-and-load command to scheduled jobs #2759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #2351, #2472] Migrate to platforms step function pattern #2506

[Issue #2351, #2472] Migrate to platforms step function pattern #2506

coilysiren commented Oct 17, 2024 •

edited

Loading

coilysiren Oct 17, 2024

chouinar Oct 17, 2024

mdragon Oct 17, 2024

coilysiren Oct 17, 2024

coilysiren Oct 17, 2024

mdragon left a comment

[Issue #2351, #2472] Migrate to platforms step function pattern #2506

[Issue #2351, #2472] Migrate to platforms step function pattern #2506

Conversation

coilysiren commented Oct 17, 2024 • edited Loading

Summary

Time to review: 5 mins

Changes proposed

Context for reviewers

Additional information

coilysiren Oct 17, 2024

Choose a reason for hiding this comment

chouinar Oct 17, 2024

Choose a reason for hiding this comment

mdragon Oct 17, 2024

Choose a reason for hiding this comment

coilysiren Oct 17, 2024

Choose a reason for hiding this comment

coilysiren Oct 17, 2024

Choose a reason for hiding this comment

mdragon left a comment

Choose a reason for hiding this comment

coilysiren commented Oct 17, 2024 •

edited

Loading