[ETL-670] Adjust cleanup job to run on `main` directory of dev env #126

BryanFauble · 2024-07-13T00:01:09Z

Problem:

We do want to run the cleanup on the main directory of the dev branch

Solution:

Updating the workflow so the job is triggered and we are able to clean the main directory

Testing:

Will be looking at the workflow run

rxu17 · 2024-07-13T01:00:50Z

.github/workflows/upload-and-deploy.yaml

This is actually making me rethink the logic of integration-test-develop as well. integration-test-develop is just copying the recent 2 weeks of exports and should just be triggering the branch's specific namespaced jobs to run on that subset of exports but it shouldn't be doing it for the main namespace in the dev project when we merge to main because main will always run on all of the data in dev to mirror what it does in prod (which it's currently doing)

I am thinking now that these are the updates we could make and it makes a bit more sense:

integration-test-develop needs the if: github.ref_name != 'main' line at the highest level of the GH job

integration-test-develop-cleanup needs the if: github.ref_name != 'main' line at the highest level of the GH job

sceptre-deploy-staging will have the update:needs: sceptre-deploy-develop

I think this will give the logic we need. That the integration test and cleanup for when we are in dev environment to only run when we are working on the namespaced branch, and doesn't run when we merge to main.

Thoughts?

See the latest comments on this PR, but I think we want the same behavior when there are changes on dev/main as any other dev/* namespace.

philerooski

I don't think this is what we want. We don't need to handle integration tests in dev/main any differently than we handle tests for our feature branch. We've already specified environment = develop on L265, so this is happening separately from our production pipeline and there's no harm in running the integration tests in the main namespace.

BryanFauble · 2024-07-15T16:59:17Z

We've already specified environment = develop on L265, so this is happening separately from our production pipeline and there's no harm in running the integration tests in the main namespace.

@philerooski From @rxu17's comments on #121 (comment) - It sounded like we did not want to run cleanup for main.

I agree that we should be running integration tests in the main namespace, but is it acceptable that we would be cleaning up the directories in the main namespace?

BryanFauble · 2024-07-15T17:30:34Z

I threw this mermaid diagram together for my sake to visualize this workflow:

flowchart LR;
  pre-commit-->upload-files-->sceptre-deploy-develop
  pre-commit-->nonglue-unit-tests-->sceptre-deploy-develop
  pre-commit-->pytest-docker-->glue-unit-tests-->sceptre-deploy-develop-->integration-test-develop
  sceptre-deploy-develop-->integration-test-develop-cleanup-->integration-test-develop
  integration-test-develop-->sceptre-deploy-staging[sceptre-deploy-staging\nif: github.ref_name == 'main']-->integration-test-staging
  sceptre-deploy-staging-->integration-test-staging-cleanup
  integration-test-staging-cleanup[integration-test-staging-cleanup\nif: github.ref_name == 'main']-->integration-test-staging

philerooski · 2024-07-15T17:48:30Z

@BryanFauble Our dev data has a permanent home in the pilot-data prefix in the input bucket. Anything in the main prefix is ephemeral and any data in this prefix in any of the buckets ought to reflect HEAD of the main branch.

rxu17 · 2024-07-15T17:55:53Z

I don't think this is what we want. We don't need to handle integration tests in dev/main any differently than we handle tests for our feature branch. We've already specified environment = develop on L265, so this is happening separately from our production pipeline and there's no harm in running the integration tests in the main namespace.

So the reason I thought we don't need to run integration tests for main namespace of develop is because we already run it for the feature_branch namespace of develop. But no nevermind, we do need integration tests for main namespace to run so that we are triggering the S3 to JSON job runs for main of develop after we merge a feature branch for main and we don't need to manually run that part.

In that case, then I think it's fine if we cleanup the main namespace of develop in both the input and intermediate data buckets. In our case, because all of our pilot data gets copied over (because we only have more than 2 weeks of exports for pilot data), we maintain the expected workflow for main namespace which is that the main namespaced jobs run on all of the input data

BryanFauble · 2024-07-15T18:08:03Z

Thanks for the feedback @rxu17 @philerooski Take a look at the latest round of changes. We should always clean the directories now in develop

rxu17 · 2024-07-15T18:16:31Z

src/scripts/manage_artifacts/clean_for_integration_test.py

@@ -56,9 +54,6 @@ def main() -> None:
    if not args.bucket_prefix or args.bucket_prefix[-1] != "/":
        raise ValueError("Bucket prefix must be provided and end with a '/'")

-    if "main" in args.bucket_prefix:


Ah I forgot about us having to remove this if we want to clean up main. Hmm, should we add another safeguard?

if dev not in args.bucket: raise ValueError("Must be using buckets in the develop environment")

OR

dev_buckets = ["recover-dev-input-data", "recover-dev-intermediate-data", "recover-dev-processed-data", "recover-dev-cloudformation"] if args.bucket not in dev_buckets: raise ValueError("Must be using buckets in the develop environment")

Overkill?

We will also be running this on the prod buckets too - But the staging directory of those buckets. I think controlling this in the workflow makes the most sense here.

We could add a safeguard for prod/main (we still want to be able to clean up prod/staging). I think this script should be agnostic to the object key, except for the aforementioned case.

How does this guard condition look?

6e36626

Sorry, my brain is not functioning right now. Yes I second what Phil said.
Looks good to me!

rxu17

LGTM! Just one comment!

sonarqubecloud · 2024-07-15T18:32:21Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

rxu17 · 2024-07-15T18:33:01Z

src/scripts/manage_artifacts/clean_for_integration_test.py

@@ -56,8 +54,10 @@ def main() -> None:
    if not args.bucket_prefix or args.bucket_prefix[-1] != "/":
        raise ValueError("Bucket prefix must be provided and end with a '/'")

-    if "main" in args.bucket_prefix:
-        raise ValueError("Cannot delete objects in the main directory")
+    if "main" in args.bucket_prefix and "recover-dev" not in args.bucket:


Adjust cleanup job to run always, but skip steps if main

4b300e3

BryanFauble requested a review from a team as a code owner July 13, 2024 00:01

BryanFauble temporarily deployed to develop July 13, 2024 00:02 — with GitHub Actions Inactive

BryanFauble temporarily deployed to develop July 13, 2024 00:11 — with GitHub Actions Inactive

BryanFauble temporarily deployed to develop July 13, 2024 00:15 — with GitHub Actions Inactive

BryanFauble temporarily deployed to develop July 13, 2024 00:16 — with GitHub Actions Inactive

rxu17 reviewed Jul 13, 2024

View reviewed changes

philerooski requested changes Jul 15, 2024

View reviewed changes

Update when cleanup is ran

4430539

BryanFauble temporarily deployed to develop July 15, 2024 18:05 — with GitHub Actions Inactive

BryanFauble changed the title ~~[ETL-670] Adjust cleanup job to run always, but skip steps if main~~ [ETL-670] Adjust cleanup job to run on main directory of dev env Jul 15, 2024

BryanFauble temporarily deployed to develop July 15, 2024 18:08 — with GitHub Actions Inactive

rxu17 reviewed Jul 15, 2024

View reviewed changes

rxu17 self-requested a review July 15, 2024 18:17

rxu17 approved these changes Jul 15, 2024

View reviewed changes

BryanFauble temporarily deployed to develop July 15, 2024 18:22 — with GitHub Actions Inactive

BryanFauble temporarily deployed to develop July 15, 2024 18:24 — with GitHub Actions Inactive

BryanFauble temporarily deployed to develop July 15, 2024 18:25 — with GitHub Actions Inactive

Add guard condition for main and non-dev bucket

6e36626

BryanFauble temporarily deployed to develop July 15, 2024 18:32 — with GitHub Actions Inactive

rxu17 reviewed Jul 15, 2024

View reviewed changes

BryanFauble temporarily deployed to develop July 15, 2024 18:35 — with GitHub Actions Inactive

BryanFauble temporarily deployed to develop July 15, 2024 18:41 — with GitHub Actions Inactive

BryanFauble temporarily deployed to develop July 15, 2024 18:43 — with GitHub Actions Inactive

BryanFauble temporarily deployed to develop July 15, 2024 18:44 — with GitHub Actions Inactive

philerooski approved these changes Jul 15, 2024

View reviewed changes

BryanFauble merged commit ae2cf3c into main Jul 15, 2024
17 checks passed

BryanFauble deleted the etl-670-adjust-dev-cleanup branch July 15, 2024 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETL-670] Adjust cleanup job to run on `main` directory of dev env #126

[ETL-670] Adjust cleanup job to run on `main` directory of dev env #126

BryanFauble commented Jul 13, 2024 •

edited

Loading

rxu17 Jul 13, 2024 •

edited

Loading

philerooski Jul 15, 2024

philerooski left a comment

BryanFauble commented Jul 15, 2024

BryanFauble commented Jul 15, 2024

philerooski commented Jul 15, 2024

rxu17 commented Jul 15, 2024 •

edited

Loading

BryanFauble commented Jul 15, 2024

rxu17 Jul 15, 2024 •

edited

Loading

BryanFauble Jul 15, 2024

philerooski Jul 15, 2024

BryanFauble Jul 15, 2024

rxu17 Jul 15, 2024 •

edited

Loading

rxu17 left a comment

sonarqubecloud bot commented Jul 15, 2024

rxu17 Jul 15, 2024

[ETL-670] Adjust cleanup job to run on main directory of dev env #126

[ETL-670] Adjust cleanup job to run on main directory of dev env #126

Conversation

BryanFauble commented Jul 13, 2024 • edited Loading

rxu17 Jul 13, 2024 • edited Loading

Choose a reason for hiding this comment

philerooski Jul 15, 2024

Choose a reason for hiding this comment

philerooski left a comment

Choose a reason for hiding this comment

BryanFauble commented Jul 15, 2024

BryanFauble commented Jul 15, 2024

philerooski commented Jul 15, 2024

rxu17 commented Jul 15, 2024 • edited Loading

BryanFauble commented Jul 15, 2024

rxu17 Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

BryanFauble Jul 15, 2024

Choose a reason for hiding this comment

philerooski Jul 15, 2024

Choose a reason for hiding this comment

BryanFauble Jul 15, 2024

Choose a reason for hiding this comment

rxu17 Jul 15, 2024 • edited Loading

Choose a reason for hiding this comment

rxu17 left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 15, 2024

Quality Gate passed

rxu17 Jul 15, 2024

Choose a reason for hiding this comment

[ETL-670] Adjust cleanup job to run on `main` directory of dev env #126

[ETL-670] Adjust cleanup job to run on `main` directory of dev env #126

BryanFauble commented Jul 13, 2024 •

edited

Loading

rxu17 Jul 13, 2024 •

edited

Loading

rxu17 commented Jul 15, 2024 •

edited

Loading

rxu17 Jul 15, 2024 •

edited

Loading

rxu17 Jul 15, 2024 •

edited

Loading