SDKQE-3617: Enable PSC linking to GCP instances#117
Conversation
|
I'm planning to remove |
| Region string `yaml:"region"` | ||
|
|
||
| _DefaultRegion string `yaml:"default-region"` | ||
| Zone string `yaml:"zone"` |
There was a problem hiding this comment.
This change appears to be a breaking config change and would need to be done as part of the config versioning system that's built into dinocluster. Separately from this though, I don't think selecting a specific zone is something that is meant to exist in the config file, but instead the region should be defined, and the specific zone would be auto-detected or auto-selected.
| vmId, _ := cmd.Flags().GetString("vm-id") | ||
|
|
||
| instanceIdGcp, _ := cmd.Flags().GetString("gcp-instance-id") | ||
| projectIdGcp, _ := cmd.Flags().GetString("gcp-project-id") |
There was a problem hiding this comment.
I think this should be something that can be automatically determined based on the instance-id? Or perhaps the project-id is actually something that should be defined in the configuration or as part of your GCP CLI configuration?
There was a problem hiding this comment.
Yes, I recently planned to remove this from setup-link command and add it to GCP CLI configuration.
| if shouldAutoConfig { | ||
| if instanceId != "" || vmId != "" { | ||
| logger.Fatal("must not specify both auto and instance-id/vm-id") | ||
| if instanceId != "" || vmId != "" || instanceIdGcp != "" { |
There was a problem hiding this comment.
What's the reason for adding an additional parameter here when GCP uses the same terminology as AWS? You should be able to determine the 'meaning' in terms of AWS vs GCP trivially by looking at the clouddeploy.ClusterInfo to know what kind of cluster this is. Ultimately the instance-id and vm-id are really just aliases of eachother for convenience.
There was a problem hiding this comment.
Oh yes, I'll make the changes
| SubnetIds: vpcInfo.SubnetworkID, | ||
| }) | ||
|
|
||
| err = helper.ExecuteBashCommand(command) |
There was a problem hiding this comment.
We intentionally avoid directly invoking other system commands from dinocluster, instead opting to implement them directly in cbdinocluster in order to ensure that cbdinocluster remains compatible with the widest variety of platforms, to be able to provide more nuanced diagnostic information when things go wrong as well as ensuring that dinocluster does not have unnecessary dependancies for users on third-party tools. The information needed to bind the private endpoint should be available from the API separate from the generated command they give you, if it does not currently do this, we should file an AV bug, since this will likely impact other automated tools like terraform and parse the command they provide in the short-term.
2 participants
No description provided.