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

chore: Stream S3 files directly to zip #2380

Merged
merged 1 commit into from
Nov 18, 2023

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Nov 6, 2023

What does this PR do?

  • A further step to try and improve the submissions process to Uniform
  • Stream S3 files directly to the zip, no need to write and then delete locally
  • I'll try this on the Pizza to check if it works, but will need to promote to staging to checkout Fargate health
  • Regardless of the actual affect on Fargate CPU usage this also seems like a sensible small simplification here I think

Copy link

github-actions bot commented Nov 6, 2023

Removed vultr server and associated DNS entries

@DafyddLlyr DafyddLlyr marked this pull request as ready for review November 6, 2023 09:15
@Mike-Heneghan
Copy link
Contributor

What does this PR do?

  • A further step to try and improve the submissions process to Uniform
  • Stream S3 files directly to the zip, no need to write and then delete locally
  • I'll try this on the Pizza to check if it works, but will need to promote to staging to checkout Fargate health
  • Regardless of the actual affect on Fargate CPU usage this also seems like a sensible small simplification here I think

Hey @DafyddLlyr

I think the code change looks good but I was struggling to figure out how to test this in the Pizza 🤔

I've got a submission service where I upload some files and send it to uniform which seems to be successful based on the network tab. Although when I look on the Hasura console for the pizza service I don't see anything in the uniform applications:

Screenshot 2023-11-08 at 11 20 51

I believe the files are uploaded to S3 but I'm trying to see the submission info so I can connect the application to uniform to an S3 bucket folder to check the files are uploaded successfully?

@jessicamcinchak
Copy link
Member

Catching up on reviews - @Mike-Heneghan easier way to test this one might be to use "Send to email", which generates the same zip containing user-uploaded files from S3 that gets sent to Uniform!

Here's how I'd do that:

  1. Add my own email to teams.submission_email for a given team in this pizza's Hasura console
  2. Create a new flow within that team - it will need a file upload & send component with "Send to email" checked at minimum
  3. Publish the flow
  4. Go through & submit via the /preview link
  5. Check your inbox - there should be an email with a /download-application-files endpoint - confirm you can download the zip and that it contains all the files you uploaded

In terms of your prior Uniform test, a couple things could have not been connecting here:

  • Only LDCs within Lambeth, Southwark, or Bucks can be submitted successfully to Uniform - even though you can "toggle it on" in any Send component - which flow were you trying?
  • Did you check the Hasura events table on the pizza - was there an error thrown for your attempted Uniform submission? These can fail a bit quietly on non-production envs unless you're looking
  • The Uniform audit table only stores the sessionId, not the actual zip - if you clone the branch, you can test Uniform-specific zip locally by commenting out this line, then finding the generated zip in temp files to preview

@DafyddLlyr DafyddLlyr force-pushed the dp/stream-directly-from-s3-to-zip branch from 4d5806a to 321b5cb Compare November 8, 2023 18:25
@DafyddLlyr
Copy link
Contributor Author

Thanks for the helpful comment above @jessicamcinchak 🙌

Testing of this has been a little on the backburner as publishing was failing due to a bug fixed earlier in the week. I've now rebased this which should make things simpler now.

Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Coming back to this one in hopes of merging before you're off next week!

I hit one issue in testing:

Might just be a rebase issue? Sorry this one has sat for awhile, happy to make time to test again today if you can similarly re-create!

@DafyddLlyr
Copy link
Contributor Author

Thanks for the heads up - will also take another look here. No real rush to move this one forward but I'll investigate and fix and then @ you if I need another review 👍

@DafyddLlyr DafyddLlyr self-assigned this Nov 17, 2023
@DafyddLlyr
Copy link
Contributor Author

@jessicamcinchak Finally got back to this one - sorry for the holdup.

The issue was that the flow wasn't published yet on the Pizza. I published and then send to email worked as expected 👍

There's a small PR here which might make this slightly easier to work out in future - I keep on hitting this issue! theopensystemslab/planx-core#199

Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Eeep sorry for silly mistake 🙈 working now, happy for this to go in!

@DafyddLlyr
Copy link
Contributor Author

Eeep sorry for silly mistake 🙈 working now, happy for this to go in!

No zero bother at all - I keep hitting the same issue! Appreciate the reviews this morning - sorry if me tagging these for review has bothered you on a day ay off 🙏

@DafyddLlyr DafyddLlyr merged commit 96adf16 into main Nov 18, 2023
12 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/stream-directly-from-s3-to-zip branch November 18, 2023 12:35
DafyddLlyr added a commit to theopensystemslab/planx-core that referenced this pull request Nov 18, 2023
Small fix that might help resolve this issue which I keep hitting!
theopensystemslab/planx-new#2380 (comment)

No real rush to get this into `planx-new`, it can come along on the next
update.
jessicamcinchak added a commit that referenced this pull request Nov 21, 2023
jessicamcinchak added a commit that referenced this pull request Nov 21, 2023
DafyddLlyr added a commit that referenced this pull request Dec 11, 2023
* chore: Stream S3 files directly to zip (#2380)

* fix: Remove /tmp prefix in zip files
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.

3 participants