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

🔨 Add wrangler.toml file #3846

Merged
merged 1 commit into from
Aug 13, 2024
Merged

🔨 Add wrangler.toml file #3846

merged 1 commit into from
Aug 13, 2024

Conversation

danyx23
Copy link
Contributor

@danyx23 danyx23 commented Aug 5, 2024

This PR adds a wrangler.toml file to the grapher repo for the pages functions. It should be a NO-OP in terms of functionality. It's added mostly so that we can use the toml file to add per-environment specific variables or bindings in the future.

Copy link
Contributor Author

danyx23 commented Aug 5, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @danyx23 and the rest of your teammates on Graphite Graphite

@owidbot
Copy link
Contributor

owidbot commented Aug 5, 2024

Quick links (staging server):

Site Admin Wizard

Login: ssh owid@staging-site-enable-wrangler-toml

SVG tester:

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

Edited: 2024-08-09 16:03:31 UTC
Execution time: 1.21 seconds

@danyx23 danyx23 marked this pull request as ready for review August 6, 2024 10:31
@danyx23 danyx23 force-pushed the enable-wrangler-toml branch from 29ba21d to e63e906 Compare August 6, 2024 14:34
@danyx23 danyx23 requested a review from marcelgerber August 6, 2024 16:19
@danyx23
Copy link
Contributor Author

danyx23 commented Aug 6, 2024

I ran deployContentPreview and checked that the thumbnail worker there still does what it should: https://enable-wrangler-toml.owid-staging.pages.dev/grapher/thumbnail/life-expectancy

I think it should be safe to merge this but when we do we should be vigilant in case there is some config weirdness in prod that isn't present in staging

@danyx23 danyx23 force-pushed the enable-wrangler-toml branch from e63e906 to f61425d Compare August 7, 2024 20:36
@danyx23 danyx23 force-pushed the enable-wrangler-toml branch 7 times, most recently from 28b2124 to ad39546 Compare August 9, 2024 14:47
Comment on lines 25 to 27
return new Response(JSON.stringify(info), {
headers: { "Content-Type": "application/json" },
})
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return new Response(JSON.stringify(info), {
headers: { "Content-Type": "application/json" },
})
return Response.json(info)

@danyx23 danyx23 force-pushed the enable-wrangler-toml branch from 26bef01 to 73fc4bd Compare August 12, 2024 11:35
@danyx23
Copy link
Contributor Author

danyx23 commented Aug 12, 2024

@marcelgerber I squashed the changes now of this PR and removed the debug code again. The settings from wrangler.toml showed up in the dashboard for the preview now which is nice - only the secrets are still managed via the dashboard (which makes sense).

On staging, some settings for dev keys were not set as secrets and with the introduction of wrangler.toml they then vanished (as they are not in wrangler.toml either). Since they should have been configured as secrets anyhow, I made sure to add the dev keys as secrets for the preview deployment in the CF dashboard. On prod, everything looks good (i.e. everything is configured as secrets except for the three vars that are set in wrangler.toml.

As we discussed, since the preview deployment to CF and the prod deploy use the same mechanism and "./dist" folder, I think this should be good to go.

I'd appreciate a quick look over it as a sanity check then we can finally merge this one :)

@danyx23 danyx23 requested a review from marcelgerber August 12, 2024 11:42
Copy link
Member

@marcelgerber marcelgerber left a comment

Choose a reason for hiding this comment

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

Nice, looks good!

Copy link
Contributor Author

danyx23 commented Aug 13, 2024

Merge activity

  • Aug 13, 12:40 PM EDT: @danyx23 started a stack merge that includes this pull request via Graphite.
  • Aug 13, 12:40 PM EDT: @danyx23 merged this pull request with Graphite.

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