Conversation
When running in an app directory, ConfigCentric now checks per-app cluster state from app-state.toml after -C flag and MIREN_CLUSTER env, but before the global active_cluster fallback. This is the same state that cluster switch saves. With the resolution logic centralized, AppCentric.LoadCluster() can drop its parallel implementation and just delegate. cluster current gets the same treatment — its ad-hoc resolution is replaced with a LoadCluster() call.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR refactors cluster resolution logic across multiple CLI commands to use a centralized LoadCluster method instead of individual config/state lookups. The config.go file now handles per-app state checking during cluster resolution, falling back to global settings when an active app's state doesn't specify a cluster. Commands including cluster, cluster_current, cluster_export, doctor, and doctor_auth are updated to call opts.LoadCluster() for cluster retrieval. The app.go command delegates to ConfigCentric.LoadCluster, while app_list.go switches from configuration loading to direct cluster loading. Tests are added to validate environment-based and app-scoped cluster resolution paths, including fallback behavior and CLI flag/environment variable overrides. 📝 Coding Plan
Comment |
Several commands were doing their own ad-hoc "-C flag then ActiveCluster()" dance instead of going through LoadCluster(). This meant they'd silently ignore per-app state and MIREN_CLUSTER in some cases. Swept through AppList, ClusterList, ClusterExportAddress, Doctor, and DoctorAuth to use the unified path. ClusterList previously duplicated the per-app state lookup manually; now it just calls LoadCluster() like everyone else.
fa8f081 to
5cfe8db
Compare
When a user runs
cluster switchin an app directory, it saves the chosen cluster to both the global config and per-app state. Commands usingAppCentric(likecluster current,deploy) would read that per-app state and target the right cluster. But commands usingConfigCentric(likeroute list,sandbox list,app list) skipped per-app state entirely and fell back to the globalactive_cluster. So you could seecluster currentreport "miren01" whileroute listsilently hits a completely different cluster.The fix teaches
ConfigCentric.LoadCluster()itself to check per-app state when running in an app directory, slotted in between the explicit overrides (-C flag, MIREN_CLUSTER env) and the global fallback. This is the minimal change that fixes the inconsistency without restructuring the command hierarchy. With the logic centralized,AppCentric.LoadCluster()drops its parallel implementation and just delegates, andcluster currentreplaces its bespoke resolution with aLoadCluster()call.A sweep of the rest of the CLI commands turned up several more that were doing their own ad-hoc resolution instead of going through
LoadCluster()—app list,cluster list,cluster export-address,doctor, anddoctor authall got converted. The only intentional exception isdeploy's interactive "add a cluster" recovery path, which correctly reads the just-added cluster directly.Note:
doctorcould use a deeper rework beyond this scope — it's doing a lot of manual wiring that doesn't quite fit theLoadCluster()model cleanly. Filing a follow-up for that.Closes MIR-832