Skip to content

Conversation

@nmdefries
Copy link
Contributor

Description

  • Since our AWS bucket has a policy enforcing who can upload and who can view, remove --acl option from the upload command.
  • Only upload prediction files, not coefficient files.
  • Check that AWS upload step uploaded the number of files that we expected; if not, error.

Changelog

  • Makefile

@nmdefries nmdefries requested a review from krivard February 23, 2023 21:14
Copy link
Contributor

@krivard krivard left a comment

Choose a reason for hiding this comment

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

👍 should work

I'm definitely squinting at bash for making this kind of operation awkward though, it seems like tests and pipes should be able to be friends

Comment on lines +92 to +94
AWS_MSG=`aws s3 cp $(USR_EXPORT_DIR) $(S3_BUCKET)/ --recursive --exclude "*" --include "*/prediction*.csv.gz"`; \
echo "$${AWS_MSG}"; \
[[ `echo "$${AWS_MSG}" | grep "upload:" | wc -l` -eq $$NUM_FILES ]] || ( echo "ERROR: Not all $${NUM_FILES} expected files were uploaded" && exit 74 ); \
Copy link
Contributor

Choose a reason for hiding this comment

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

thought (nonblocking): it really feels like there should be a way to do this with tee or process substitution instead of variables and echo

I played with it for a while and the closest I got was teeing to stderr, which would probably be excluded from the log and that's not what we want... and your way is definitely more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's awkward to have the file upload running as a side effect.

There are some suggestions online to tee to /dev/tty or $(tty) but I can't actually get this to work on my own computer. Something to look into.

Copy link
Contributor

Choose a reason for hiding this comment

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

that could work when running from a terminal, but normal pipeline operations typically run headless alas

@nmdefries
Copy link
Contributor Author

This is ready to merge

@krivard krivard merged commit ec9a30f into main Mar 1, 2023
@krivard krivard deleted the ndefries/backfill/aws-upload-settings branch March 1, 2023 17:19
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