-
Notifications
You must be signed in to change notification settings - Fork 123
feat(cdn): add KV_BASE_URL for CDN worker + add compatibility layer #7202
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @noghartt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the CDN worker and server configurations by introducing a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a KV_BASE_URL environment variable for the CDN worker to allow custom KV storage endpoints, which is particularly useful for self-hosted setups. The changes span across the CDN worker and the server packages, adding the necessary configuration, type definitions, and logic to use the new variable while maintaining backward compatibility with a default URL.
My review has identified a critical syntax error that needs to be fixed, along with some medium-severity issues related to documentation accuracy, code maintainability (magic strings), and typos in comments. Overall, the changes are well-structured and address the intended feature.
| const cacheKey = new Request( | ||
| [ | ||
| 'https://key-cache.graphql-hive.com', | ||
| args.kvStorageBaseUrl ?? 'https://key-cache.graphql-hive.com', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default KV storage base URL 'https://key-cache.graphql-hive.com' is hardcoded here and again on line 155. To avoid magic strings and ensure consistency, it's better to define this as a constant at the top of the file and reuse it in both places.
For example:
const DEFAULT_KV_STORAGE_BASE_URL = 'https://key-cache.graphql-hive.com';Then you can use it here and on line 155.
packages/services/server/README.md
Outdated
| | `S3_MIRROR_PUBLIC_URL` | No | The public URL of the S3, in case it differs from the `S3_ENDPOINT`. | `http://localhost:8083` | | ||
| | `CDN_API` | No | Whether the CDN exposed via API is enabled. | `1` (enabled) or `0` (disabled) | | ||
| | `CDN_API_BASE_URL` | No (Yes if `CDN_API` is set to `1`) | The public base url of the API service. | `http://localhost:8082` | | ||
| | `CDN_API_KV_BASE_URL` | No (**Optional** if `CDN_API` is set to `1`) | The base URL for the KV for OIDC related endpoints. | `https://key-cache.graphql-hive.com` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true, the endpoint is not related to OIDC, only for caching
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, thought that there's some kind of relationship with the endpoint per se here.
packages/services/server/README.md
Outdated
| | `COMMERCE_ENDPOINT` | **Yes** | The endpoint of the commerce service. | `http://127.0.0.1:4012` | | ||
| | `CDN_CF` | No | Whether the CDN is enabled. | `1` (enabled) or `0` (disabled) | | ||
| | `CDN_CF_BASE_URL` | No (**Yes** if `CDN` is `1`) | The base URL of the cdn. | `https://cdn.graphql-hive.com` | | ||
| | `CDN_CF_KV_BASE_URL` | No (**Optional** if `CDN` is `1`) | The base URL for the KV using CloudFlare provider | `https://key-cache.graphql-hive.com` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is true, it could be improved.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
As a general question: what are the implications if you deploy this to your own cloudflare, with |
From the tests I did in my self-hosted solution, it didn't work. So what I did was changing it in a similar way I did on this PR (without the conditional). But not sure if you guys are using this URL for the caching yet or there's something new, I left it as the fallback, just in case to avoid any breaking change (even because there's a usage of it on the |
Background
Fix #7036
This PR adds a new env for
KV_BASE_URLwhich will let both CF CDN worker + API custom providers work on self-hosted setup for CDN.Also, it adds a guarantee that it won't create any breaking change considering the old value.
Description
We changed these services:
package/services/cdn-worker: add a new envKV_BASE_URL+ expose a new field on functioncreateIsKeyValidto let the user pass any string value to be used by artifact handler.packages/services/serverto, when using API provider for CDN, it let the user insert any KV, for compatibility with their setup (if on self-hosted).Checklist