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

🔨 store grapher configs in R2 when edited in the admin #3827

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

danyx23
Copy link
Contributor

@danyx23 danyx23 commented Jul 31, 2024

This PR does 2 things at the high level: it stores grapher configs in R2 when they are edited in the admin, and it adds a tool for syncing the content of the chart_configs table into R2 (adding/updating missing ones and deleting superfluous ones).

Charts are stored in R2 in two folders for now: one that is used for all configs addressed by chart UUID, and one addressed by slug for only published standalone charts.

This PR adds a hash of the full config in the chart_configs table. I started out using a SHA-1 and was happy that R2 supports those in addition to MD5, only to discover later on when I wrote the sync tool that the S3 API list operation has no support for hashes other than MD5. Since the main point of the hash is facilitate efficient comparison between set of configs in the DB and those in R2 I then rewrote the hash to be md5. The hash is stored in base64 encoding since this is also what is used and returned in most api calls (the only exception is the ETAG where a hex serialization in double quotes is used 🤷)

We already have a key configured for interacting with R2 so I renamed the settings related to this key to have a generic (non-image specific) name.

To test this:

  • set the R2_ACCESS_KEY_ID and R2_SECRET_ACCESS_KEY to a personal CF R2 token that you generate. If you already did this previously for images (i.e. if your .env file already has IMAGE_HOSTING_R2_ACCESS_KEY_ID) then rename these two keys AND make sure that your personal key has access to the owid-grapher-configs-staging bucket (CF dashboard -> R2 -> manage api tokens in the top right)
  • set GRAPHER_CONFIG_R2_BUCKET to owid-grapher-configs-staging and GRAPHER_CONFIG_R2_BUCKET_PATH to devs/YOURNAME

Then when you interact with the admin and save graphers, you should see these charts show up as json files in the R2 bucket in the owid-grapher-configs-staging bucket.

Copy link
Contributor Author

danyx23 commented Jul 31, 2024

@danyx23 danyx23 force-pushed the inherit-defaults branch 2 times, most recently from d4d178b to f68729d Compare August 5, 2024 17:25
@danyx23 danyx23 changed the title 🔨 rename R2 settings to generic names 🔨 store grapher configs in R2 when edited in the admin Aug 7, 2024
@owidbot
Copy link
Contributor

owidbot commented Aug 7, 2024

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-grapher-configs-in-r2

SVG tester:

Number of differences (default views): 35 (9a4c56) ❌
Number of differences (all views): 0 ✅

Edited: 2024-09-09 12:02:51 UTC
Execution time: 1.20 seconds

Copy link
Contributor Author

@danyx23 danyx23 left a comment

Choose a reason for hiding this comment

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

Do we have a good sense of how we can reconstruct chart revisions if we're only serializing the patch?

Without inheritance it's the same as before but with inheritance it's true that this becomes a bit more tricky. Let's chat about it a bit today. I think storing the full config wouldn't be better though.

@sophiamersmann sophiamersmann force-pushed the grapher-configs-in-r2 branch 2 times, most recently from 1321bc7 to 8b8ab75 Compare September 3, 2024 10:24
@danyx23 danyx23 force-pushed the grapher-configs-in-r2 branch 2 times, most recently from 56646bf to 065c651 Compare September 3, 2024 14:43
@sophiamersmann sophiamersmann changed the base branch from indicator-chart-editor to inherit-indicator-settings September 3, 2024 15:12
@sophiamersmann sophiamersmann force-pushed the grapher-configs-in-r2 branch 4 times, most recently from 5187516 to 6bacddb Compare September 4, 2024 12:59
@sophiamersmann sophiamersmann force-pushed the grapher-configs-in-r2 branch 3 times, most recently from 5db2dfb to e097788 Compare September 5, 2024 07:41
Base automatically changed from inherit-indicator-settings to master September 5, 2024 08:59
@danyx23 danyx23 force-pushed the grapher-configs-in-r2 branch 2 times, most recently from 691b7ea to 47ccc78 Compare September 5, 2024 12:13
@danyx23 danyx23 force-pushed the grapher-configs-in-r2 branch 2 times, most recently from b6ffcc0 to c32a418 Compare September 9, 2024 09:33
Copy link
Contributor Author

danyx23 commented Sep 9, 2024

Merge activity

  • Sep 9, 10:35 AM EDT: @danyx23 started a stack merge that includes this pull request via Graphite.
  • Sep 9, 10:35 AM EDT: @danyx23 merged this pull request with Graphite.

@danyx23 danyx23 merged commit 85e049a into master Sep 9, 2024
22 checks passed
@danyx23 danyx23 deleted the grapher-configs-in-r2 branch September 9, 2024 14:35
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