-
Notifications
You must be signed in to change notification settings - Fork 25
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
Move info into tc status #433
Move info into tc status #433
Conversation
the changes after we migrate to kubeconfig-based connections. Add fields to the ToolchainCluster.Status that will advertise info from kubeconfig.
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.
Looks good 👍
I have only one small comment.
Co-authored-by: Francisc Munteanu <[email protected]>
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.
Thanks for the additional notes and comments on the fields 👍
@@ -34,20 +34,33 @@ const ( | |||
type ToolchainClusterSpec struct { | |||
// The API endpoint of the member cluster. This can be a hostname, | |||
// hostname:port, IP or IP:port. | |||
// | |||
// Be aware that this field is going to be replaced with | |||
// the Status.APIEndpoint in the future. | |||
APIEndpoint string `json:"apiEndpoint"` |
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.
btw, you could have made this field optional as well, so you can drop populating it before you remove it from the API
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.
Not sure what's better from the client's POV. A hard error from using a removed field or a soft error caused by empty values.
Description
This introduces new fields into the ToolchainCluster status:
apiEndpoint
andoperatorNamespace
are introduced into the status because these fields will be read from the kubeconfig contained in the ToolchainCluster secret instead of explicitly being set in the ToolchainCluster's spec.Checks
Did you run
make generate
target? yesDid
make generate
change anything in other projects (host-operator, member-operator)? yesIn case of new CRD, did you the following? no new CRD
In case other projects are changed, please provides PR links.