fix(commands): honor --endpoints in talosconfig regenerate flow#202
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWhen regenerating a talosconfig without an existing config file, the endpoint initialization now resolves from ChangesTalosconfig Endpoint Initialization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates pkg/commands/talosconfig.go to utilize resolveTalosconfigEndpoints when generating a new configuration, ensuring that the --endpoints flag is correctly respected and fixing a bug where it was previously ignored. The reviewer suggests refactoring resolveTalosconfigEndpoints into a more central location to improve modularity and reduce coupling between command files.
| // here, `talm talosconfig --endpoints X` on a project with | ||
| // no prior talosconfig would silently discard the operator's | ||
| // flag — the same bug class init had at the assignment site. | ||
| newConfig.Contexts[clusterName].Endpoints = resolveTalosconfigEndpoints(GlobalArgs.Endpoints) |
There was a problem hiding this comment.
While reusing resolveTalosconfigEndpoints is a great way to fix this bug and maintain consistency, the function itself seems generally useful for handling talosconfig endpoints. Currently, it resides in init.go, which creates a dependency from talosconfig.go to init.go.
To improve modularity and make it clear that this is a shared helper, consider moving resolveTalosconfigEndpoints to a more central location within the commands package, such as root.go or a new helpers.go file. This would make the code organization cleaner and prevent tight coupling between command-specific files.
regenerateTalosconfig at pkg/commands/talosconfig.go:192 hit the
same silent-discard pattern as init flow: when no old talosconfig
existed, the assignment site hardcoded defaultLocalEndpoint and
discarded the operator's --endpoints flag. Reuse the
resolveTalosconfigEndpoints helper from init.go so the two
talosconfig-seeding sites stay in sync. The old-config path
(preserving existing context endpoints) is unchanged.
The helper-level contract is already pinned by
TestResolveTalosconfigEndpoints_{GlobalNonEmpty_UsesGlobal,
GlobalEmpty_UsesLoopbackFallback, ReturnsCopy, MultipleEndpoints};
the visible call site keeps both seeding paths under one helper
so a future regression where someone re-introduces a hardcoded
loopback at either site surfaces against the same test surface.
Signed-off-by: Aleksei Sviridkin <f@lex.la>
lexfrei
force-pushed
the
fix/talosconfig-honor-endpoints
branch
from
0fb60a6 to
1191b22
Compare
1 participant
What
Apply the same
--endpointsfix from PR #201 (init flow) to the parallel assignment site inregenerateTalosconfig.Why
Thorough audit of similar bug patterns across the project surfaced one more silent-flag-discard at
pkg/commands/talosconfig.go:192. TheregenerateTalosconfigflow, when called on a project with no existing talosconfig (e.g. operator runstalm talosconfigto regenerate certs but the original file is gone), hardcodeddefaultLocalEndpointand silently discarded--endpoints. Same bug class as #9341 (init flow); same fix.Other audited sites:
charts/talm/templates/_helpers.tpl::talm.discovered.bond_config(v1.11 nested bond form): identical empty-slaves shape as the multidoc bug, but not reachable today because the Talos auto-bond-stub behaviour is a 1.13 link-controller addition while the v1.11 nested form is gated ontalosVersion < v1.12. Out of scope.BridgeConfigempty-ports rendering: Talos schema validates onlynamefor bridges, so emptylinks:does not produce an apply-reject (unlike bond which requires ≥1 link). Operator noise only, not a bug. Out of scope.pkg/commands/kubeconfig_handler.go:134: correctly checkslen(GlobalArgs.Endpoints) > 0before consuming. Not affected.pkg/commands/apply.goPreRunE: seedsdefaultLocalEndpointonly whenGlobalArgs.Endpointsis empty, with a defensive reset between files. Not affected (the pattern is correct).Approach
Reuse the existing
resolveTalosconfigEndpointshelper frominit.goso the two talosconfig-seeding sites stay in sync. The old-config path (preserving existing context endpoints) is unchanged.Tests
The helper-level contract is already pinned by:
TestResolveTalosconfigEndpoints_GlobalNonEmpty_UsesGlobalTestResolveTalosconfigEndpoints_GlobalEmpty_UsesLoopbackFallbackTestResolveTalosconfigEndpoints_ReturnsCopyTestResolveTalosconfigEndpoints_MultipleEndpointsThe visible call site keeps both seeding paths under one helper so a future regression where someone re-introduces a hardcoded loopback at either site surfaces against the same test surface.
Local CI:
go test ./...clean,golangci-lint run ./...0 issues.Summary by CodeRabbit