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

Debug daily metrics GHA #167

Merged
merged 5 commits into from
Sep 17, 2024
Merged

Debug daily metrics GHA #167

merged 5 commits into from
Sep 17, 2024

Conversation

e-belfer
Copy link
Member

@e-belfer e-belfer commented Sep 16, 2024

Overview

What problem does this address?
Fixes problems with the archiving action created in #162.

What did you change in this PR?

  • Update Github API token to have correct permissions
  • Update GCS permissions configuration
  • Fix typos and job name

Testing

How did you make sure this worked? How can a reviewer verify this?
Run the save-daily-metrics.yml workflow and verify that it works as expected.

To-do list

Tasks

Preview Give feedback

@e-belfer e-belfer self-assigned this Sep 16, 2024
@e-belfer e-belfer added kaggle Relating to Kaggle usage metrics github Relating to Github usage metrics labels Sep 16, 2024
@e-belfer e-belfer marked this pull request as ready for review September 17, 2024 13:40
@e-belfer e-belfer requested review from bendnorman and jdangerx and removed request for bendnorman September 17, 2024 13:41
Copy link
Member

@jdangerx jdangerx left a comment

Choose a reason for hiding this comment

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

:shipit:!! but also I have some questions about the various auth changes. Nothing blocking, just curious why they had to be done

@@ -22,7 +22,7 @@ jobs:
with:
environment-file: environment.yml
cache-environment: true
ondarc: |
Copy link
Member

Choose a reason for hiding this comment

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

-_- my eyes completely glazed over this during the first review

workload_identity_provider: "projects/345950277072/locations/global/workloadIdentityPools/gh-actions-pool/providers/gh-actions-provider"
service_account: "pudl-usage-metrics-etl@catalyst-cooperative-pudl.iam.gserviceaccount.com"
create_credentials_file: true
credentials_json: "${{ secrets.GCP_USAGE_METRICS_ARCHIVER_KEY }}"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, sure, but what was the issue with WIF?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was done to give this archiver write access and to keep the ETL account as having read-only access to the archived files.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense! Do you have a new service account now? Is it in Terraform / if not, do you want help putting it in Terraform?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me move this question into the living terraform PR (catalyst-cooperative/pudl#3841), as @bendnorman set up the account and would know better.

.github/workflows/save_daily_metrics.yml Show resolved Hide resolved
@e-belfer e-belfer merged commit 3074ae0 into main Sep 17, 2024
6 checks passed
@e-belfer e-belfer deleted the debug-daily-metrics branch September 17, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github Relating to Github usage metrics kaggle Relating to Kaggle usage metrics
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants