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.

Sign up for GitHub

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

Jump to bottom

Update grapher configs in R2 when variable config changes #4096

Merged
merged 4 commits into from
Nov 1, 2024
Merged

Update grapher configs in R2 when variable config changes #4096

merged 4 commits into from
Nov 1, 2024

Conversation

rakyi
Copy link
Contributor

@rakyi rakyi commented Oct 31, 2024

  • Fix missing updates of configs in R2 after we update the configs in the DB on variable grapher config upsert/deletion in the API
  • Update mdim view grapher configs on variable grapher config upsert/deletion
  • Trigger baking if a published mdim config is created/updated

@rakyi rakyi requested a review from danyx23 October 31, 2024 08:30
@rakyi rakyi requested review from marcelgerber and sophiamersmann and removed request for marcelgerber October 31, 2024 08:33
@rakyi
Copy link
Contributor Author

rakyi commented Oct 31, 2024

@sophiamersmann since you worked on the related code recently, could you take a look also? Feel free to ignore the mdim specifics.

@owidbot
Copy link
Contributor

owidbot commented Oct 31, 2024 •
edited
Loading

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-update-r2-configs

SVG tester:

Number of differences (default views): 0 ✅
Number of differences (all views): 0 ✅

Edited: 2024-10-31 14:19:59 UTC
Execution time: 1.27 seconds

sophiamersmann
sophiamersmann approved these changes Oct 31, 2024
View reviewed changes
Copy link
Member

@sophiamersmann sophiamersmann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I can't speak to the mdim specifics, but on a high level, this makes sense to me 👍🏻

If you wanted to, you could add a couple of db tests. This has saved me a few times while working on this stuff :) Up to you, though!

@rakyi
Copy link
Contributor Author

rakyi commented Oct 31, 2024

Thanks! I'll try adding a few tests. After one of the existing ones failed in CI, I now see we already have some, so will start there.

@rakyi rakyi force-pushed the update-r2-configs branch 4 times, most recently from 74e53ae to 85546ed Compare October 31, 2024 13:59
danyx23
danyx23 approved these changes Nov 1, 2024
View reviewed changes
Copy link
Contributor

@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.

I had some tiny notes but they are all optional - looks nice!

adminSiteServer/apiRouter.ts Outdated Show resolved Hide resolved
db/model/Variable.ts Show resolved Hide resolved
db/model/Variable.ts
UPDATE chart_configs
SET
full = ?,
updatedAt = ?
Copy link
Contributor

Choose a reason for hiding this comment

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

UpdatedAt should auto-update - I would remove the explicit update from here unless there is a good reason to do it manually?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied and tweaked existing code, and this is what we did there already. Checked just now and updatedAt is not set to auto-update, and I'm not sure why. @sophiamersmann do you remember why we set it manually?

Copy link
Member

Choose a reason for hiding this comment

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

I initially switched to auto-update and then switched back to manual updates when Mojmir noticed that the updatedAt of the chart and the updatedAt of the config were out of sync for some charts. If it’s automatic updates then that can happen when you run a migration on one of the tables, for example. I don’t recall why Mojmir needed these to be the same, chart-diff maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I vaguely remember that. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok cool, if there is a reason that that is fine. Could you just add a comment (can just quote the vague statement of Sophia even :) ) so that we don't accidentally delete this in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, added comments.

Base automatically changed from new-mdim-configs to master November 1, 2024 14:43
@rakyi
Update grapher configs in R2 when variable config changes
fa01554 
* Fix missing updates of configs in R2 after we update the configs in
  the DB on variable grapher config upsert/deletion in the API
* Update mdim view grapher configs on variable grapher config
  upsert/deletion
* Trigger baking if a published mdim config is created/updated
@rakyi
Update API tests to include mdims
142355f
@rakyi
Check the feature flag before baking multi-dim
a981221
@rakyi
Explain why we set updatedAt manually
2543b45
@rakyi rakyi merged commit fc61042 into master Nov 1, 2024
14 of 15 checks passed
@rakyi rakyi deleted the update-r2-configs branch November 1, 2024 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danyx23 danyx23 danyx23 approved these changes

@sophiamersmann sophiamersmann sophiamersmann approved these changes

Assignees

@rakyi rakyi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rakyi @owidbot @danyx23 @sophiamersmann