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

Fix socrata upload actionworkflow #594

Merged
merged 21 commits into from
Oct 1, 2024
Merged

Conversation

wrridgeway
Copy link
Member

@wrridgeway wrridgeway commented Sep 11, 2024

This github workflow is currently failing, this PR is an attempt to fix it.

BEFORE MERGING

  • Remove limits from sql queries
  • Change parcel universe asset id to actual open data portal asset id

@wrridgeway wrridgeway self-assigned this Sep 11, 2024
@wrridgeway wrridgeway linked an issue Sep 11, 2024 that may be closed by this pull request
@wrridgeway wrridgeway marked this pull request as ready for review September 19, 2024 19:21
@wrridgeway wrridgeway requested a review from a team as a code owner September 19, 2024 19:21
@wrridgeway
Copy link
Member Author

wrridgeway commented Sep 19, 2024

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:

  • I had @dfsnow add an AWS_REGION variable to the repo - was this superfluous? Is there already an AWS_DEFAULT_REGION variable?
  • I have our s3 staging directory hard-coded because I don't know the variable name for it.
  • Do I have superfluous workflow steps?

Copy link
Contributor

@jeancochrane jeancochrane left a 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!

.github/workflows/socrata_upload.yaml Outdated Show resolved Hide resolved
.github/workflows/socrata_upload.yaml Outdated Show resolved Hide resolved
@@ -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/",
Copy link
Contributor

@jeancochrane jeancochrane Sep 19, 2024

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?

Copy link
Member Author

@wrridgeway wrridgeway Sep 30, 2024

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?

Copy link
Member Author

@wrridgeway wrridgeway Sep 30, 2024

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?

@wrridgeway wrridgeway merged commit f7ef7fc into master Oct 1, 2024
9 checks passed
@wrridgeway wrridgeway deleted the 593-fix-socrata-upload-action branch October 1, 2024 21:02
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.

Fix socrata upload action
2 participants