feat(infra): migrate to standalone keyfunder image#7721
Conversation
|
c68a7d6 to
38e44d7
Compare
b73e5cb to
3e28521
Compare
1b8c30e to
86722d9
Compare
3e28521 to
ab54729
Compare
ab54729 to
fda02b8
Compare
4fdbf68 to
0924019
Compare
0924019 to
74ae37a
Compare
d7cf85d to
affd9b6
Compare
74ae37a to
daa7907
Compare
affd9b6 to
c18278d
Compare
daa7907 to
35d0a0a
Compare
c18278d to
d5cc001
Compare
35d0a0a to
024a26d
Compare
5f12aae to
f65320b
Compare
0897652 to
b1572fb
Compare
f65320b to
992f37b
Compare
38cbb55 to
d3e450c
Compare
542cd49 to
6d88f89
Compare
6d88f89 to
b29a170
Compare
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughRefactors Docker image naming/lookup into typed structures, adds registry-commit awareness to KeyFunder deployment, replaces address ExternalSecret with a ConfigMap-backed KeyFunder config, removes the long funder orchestration script, and updates Helm templates and infra scripts to use the new key-funder image and registry flow. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@typescript/infra/src/funding/key-funder.ts`:
- Around line 56-78: helmValues constructs registryUri using
DEFAULT_GITHUB_REGISTRY and this.registryCommit which can be an empty string
causing an invalid URI; update helmValues to guard against empty/undefined
this.registryCommit (or derive registryUri only when present) and return either
registryUri undefined or omit registryUri from the returned hyperlane object
when it's falsy, ensuring downstream consumers of helmValues (e.g., code
expecting registryUri) handle the optional value; locate helmValues, the
registryCommit property, DEFAULT_GITHUB_REGISTRY constant, and
generateKeyfunderYaml to implement the conditional construction and adjust the
returned object accordingly.
🧹 Nitpick comments (6)
typescript/infra/src/config/funding.ts (1)
63-102: Solid validation schema, one edge case to be aware of.The refinement only validates when both multipliers are provided, which means someone could set just
targetMultiplierwithouttriggerMultiplier(or vice versa). That might be fine if the keyfunder handles partial configs gracefully with defaults, but worth double-checking that downstream code doesn't assume "if one is set, both are set."typescript/infra/src/utils/rpcUrls.ts (1)
502-505: Empty string for registryCommit - works but could be cleaner.The comment explains why, which is good. This ties back to my earlier note about
helmValues()- whenregistryCommitis empty, the generatedregistryUriwill be malformed. For the refresh use case this might not matter ifhelmValues()isn't called, but it's a bit of a code smell to pass invalid data and rely on not using it.Consider whether
registryCommitshould be optional in the type signature, withundefinedexplicitly meaning "not needed."typescript/infra/scripts/funding/deploy-key-funder.ts (2)
15-18: Add error handling for missing .registryrc file, would ya?If the
.registryrcfile doesn't exist in the monorepo root, this'll throw an uncaught exception that's about as helpful as a screen door on a submarine. Consider wrapping this in a try-catch or checking file existence first.🛠️ Suggested improvement
-function readRegistryRc(): string { - const registryRcPath = join(getMonorepoRoot(), '.registryrc'); - return readFileSync(registryRcPath, 'utf-8').trim(); -} +function readRegistryRc(): string { + const registryRcPath = join(getMonorepoRoot(), '.registryrc'); + try { + return readFileSync(registryRcPath, 'utf-8').trim(); + } catch (error) { + throw new Error( + `Failed to read registry version from ${registryRcPath}. Ensure .registryrc exists in the monorepo root.`, + ); + } +}
54-62: Empty input could slip through like a sneaky onion.If the user types nothing and hits enter,
registryCommitbecomes an empty string. WhilevalidateRegistryCommitmight catch this, it'd be friendlier to validate the input isn't empty right here or use a default/required pattern in the input prompt.🧅 Optional: Add input validation
if (shouldOverride) { registryCommit = await input({ message: 'Enter the registry version to use (can be a commit, branch or tag):', + validate: (value) => value.trim() !== '' || 'Registry version cannot be empty', }); }typescript/infra/helm/key-funder/templates/env-var-external-secret.yaml (1)
23-25: Note: Only the first RPC endpoint is used per chain.The template extracts just
index ... 0from the RPC array. If there's failover logic needed, it'd have to be handled in the keyfunder application itself rather than at the config level. Just somethin' to keep in the back of yer mind if RPC reliability becomes an issue.typescript/infra/helm/key-funder/templates/cron-job.yaml (1)
25-42: Quote templated env values to avoid YAML parsing surprises.
REGISTRY_URIandPROMETHEUS_PUSH_GATEWAYoften include://; using| quotekeeps Helm/YAML happy and avoids edge cases.♻️ Proposed tweak
- - name: REGISTRY_URI - value: {{ .Values.hyperlane.registryUri }} + - name: REGISTRY_URI + value: {{ .Values.hyperlane.registryUri | quote }} ... - name: PROMETHEUS_PUSH_GATEWAY - value: {{ .Values.hyperlane.prometheusPushGateway }} + value: {{ .Values.hyperlane.prometheusPushGateway | quote }}
🐳 Monorepo Docker Image Built Successfully
Full image paths |
⚙️ Node Service Docker Images Built Successfully
Full image paths |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7721 +/- ##
=======================================
Coverage 77.02% 77.02%
=======================================
Files 117 117
Lines 2651 2651
Branches 244 244
=======================================
Hits 2042 2042
Misses 593 593
Partials 16 16
🚀 New features to boost your workflow:
|
Summary
Migrates the keyfunder Kubernetes deployment from the monorepo image to the standalone
@hyperlane-xyz/keyfunderpackage, deleting 1,207 lines of legacy code.Motivation
With the standalone keyfunder package in #7720, we can now:
Architecture Change
Changes
Deleted
scripts/funding/fund-keys-from-deployer.ts(1,207 lines) - replaced by standalone serviceHelm Chart Updates (
helm/key-funder/)cron-job.yamlconfigmap.yamlenv-var-external-secret.yamlHYP_KEYandRPC_URL_*addresses-external-secret.yamlInfrastructure Code
docker.tsKEYFUNDERtoDockerImageReposkey-funder.tsKeyFunderHelmManagergenerates YAML config from env contextdeploy-key-funder.tsfunding.tsconfigsDockerImageRepos.KEYFUNDERExternalSecrets Pattern
Secrets are fetched from GCP Secret Manager and injected as environment variables:
Testing
pnpm -C typescript/infra build # Then deploy via: pnpm -C typescript/infra run deploy-key-funderDependencies
Summary by CodeRabbit
Infrastructure Updates
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.