-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix socrata upload actionworkflow #594
Conversation
I'm marking this as ready for review since the workflow and script are now working as anticipated, but I have some questions about it that need to be addressed before it's merged in:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple questions below, but no serious concerns from my end!
@@ -12,7 +12,7 @@ | |||
|
|||
# Connect to Athena | |||
cursor = connect( | |||
s3_staging_dir=str(os.getenv("AWS_ATHENA_S3_STAGING_DIR")) + "/", | |||
s3_staging_dir="s3://ccao-athena-results-us-east-1/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your comment seems to suggest this wasn't working before:
I have our s3 staging directory hard-coded because I don't know the variable name for it.
But it also seems like you were passing it in properly here:
AWS_ATHENA_S3_STAGING_DIR: ${{ vars.AWS_ATHENA_S3_STAGING_DIR }} |
What behavior were you seeing that made you feel like you needed to hardcode it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed like github couldn't find the variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeancochrane This still causes an error for some reason... to you know if the github var actually exists?
This github workflow is currently failing, this PR is an attempt to fix it.
BEFORE MERGING