-
Notifications
You must be signed in to change notification settings - Fork 166
👷 do not rely on hardcoded list of DCs #4037
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
Conversation
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage 🔗 Commit SHA: 2f8cb69 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
9e7570b to
247005e
Compare
8611867 to
3517742
Compare
Bundles Sizes Evolution
🚀 CPU PerformancePending... 🧠 Memory PerformancePending... |
f018b53 to
931ed9e
Compare
0dfbe94 to
1f23c10
Compare
…tching Replace the ddtool command-line tool with direct fetch calls to the runtime-metadata-service API. This enables the scripts to work in CI environments where ddtool is not available. Key changes: - Refactored datacenter.ts to use fetchHandlingError with Vault token authentication instead of ddtool command - Made all datacenter functions async with lazy initialization - Added comprehensive test coverage for datacenter module
… datacenter deployments
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38ec673ee6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
scripts/lib/datacenter.ts
Outdated
| let cachedDatacenters: Record<string, Datacenter> | undefined | ||
|
|
||
| async function getAllDatacentersMetadata(): Promise<Record<string, Datacenter>> { | ||
| if (cachedDatacenters) { | ||
| return cachedDatacenters | ||
| } |
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.
nitpick: calling getAllDatacentersMetadata multiple times synchronously won't leverage the cache.
| let cachedDatacenters: Record<string, Datacenter> | undefined | |
| async function getAllDatacentersMetadata(): Promise<Record<string, Datacenter>> { | |
| if (cachedDatacenters) { | |
| return cachedDatacenters | |
| } | |
| let cachedDatacenters: Promise<Record<string, Datacenter>> | undefined | |
| async function getAllDatacentersMetadata(): Promise<Record<string, Datacenter>> { | |
| if (cachedDatacenters) { | |
| return cachedDatacenters | |
| } | |
| cachedDatacenters = fetchDatacentersFromRuntimeMetadataService().then(...) |
scripts/deploy/lib/testHelpers.ts
Outdated
| return Promise.resolve({ | ||
| ok: true, | ||
| json: () => Promise.resolve({}), | ||
| } as unknown as Response) |
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.
nitpick: it could be nice to have a createMockResponse function with defaults, like this:
function createMockResopnse({ status = 200, json }: { status?: number, json?: any }) {
return {
ok: status < 300,
status,
json: () => Promise.resolve(json),
text: () => Promise.resolve(JSON.stringify(json))
}
}
scripts/lib/datacenter.ts
Outdated
| export async function getAllMinorDcs(): Promise<string[]> { | ||
| return (await getAllDatacenters()).filter((dc) => !MAJOR_DCS.includes(dc) && !dc.startsWith('pr')) | ||
| } |
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.
suggestion: maybe introduce something like:
enum DatacenterType {
// ... what is a minor dc ...
Minor,
// ... what is a major dc ..
Major,
// ... what is a private region ...
PrivateRegion
}
return getDatacenterType(name: string) {
// ...
}
scripts/lib/datacenter.ts
Outdated
| interface Datacenter { | ||
| name: string | ||
| site: string | ||
| } |
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.
suggestion: why not exposing this type more, so we don't need call getSite as much and things would be a bit less async
name could be the shortName, as we never use the full name anywhere?
Also (referring to my previous comment) we could add the type:
| interface Datacenter { | |
| name: string | |
| site: string | |
| } | |
| interface Datacenter { | |
| name: string | |
| site: string | |
| type: DatacenterType | |
| } |
Motivation
We want to avoid hardcoding the list of datacenter in the release process. This will remove the need for creating a PR for new DCs.
For this we discover the list of DC and their corresponding site using
Runtime Metadata Service.ddtoolPR stacked on top of #3973
Changes
datacentervsuploadPathand try to make things more clearsiteByDatacenterin favor of addtool commanda call to Runtime Metadata ServiceTodos
install ddtool on the CI (or find another way)used Runtime Metadata ServiceTest instructions
Checklist