get connections from k8 #40
Conversation
backend/.env.dev
Outdated
| @@ -8,3 +8,4 @@ WAIT_TIME_SECONDS=20 | |||
|
|
|||
| AUTH0_BASEURL=https://auth.nucleuscloud.dev | |||
| AUTH0_AUDIENCE=https://api.nucleuscloud.com | |||
| K8_NAMESPACE=neosync | |||
There was a problem hiding this comment.
can this be something a little more descriptive that makes it clear exactly what it's purpose is?
Something like job config namespace or job namespace. something like that.
There was a problem hiding this comment.
Separately, I think we can get rid of the .env.dev stuff since we are using the Helm chart to populate everything. Downside is this can't be run on baremetal without it. Which maybe youre currently relying on.
| input neosyncdevv1alpha1.SqlConnection, | ||
| secret corev1.Secret, |
There was a problem hiding this comment.
I think you want these passed in as pointers.
Especially since the secret is technically optional
| // ) *mgmtv1alpha1.ConnectionConfig { | ||
| // return nil | ||
| // } | ||
| url := string(secret.Data["url"]) |
There was a problem hiding this comment.
Few things here:
- What if the value is defined directly in the input (sqlconnection)? secret might not be needed.
- The key for the secret is defined in the SqlConnection and might not be
url.
| // return nil | ||
| // } | ||
| url := string(secret.Data["url"]) | ||
| connCfg, err := conn_utils.ParsePostgresUrl(url) |
There was a problem hiding this comment.
this works as long as we only support postgres. You probably dont want to make this check unless the SqlConnection.Spec.Driver is postgres.
| k8Namespace := viper.GetString("K8_NAMESPACE") | ||
| if k8Namespace == "" { | ||
| return nil, fmt.Errorf("must provide K8_NAMESPACE in environment") | ||
| } |
There was a problem hiding this comment.
Idk if this really belongs here. The namespace is used for R/W the JobConfigs and such and is more bizz logic.
Also, maybe if it's not provided we just default to default? That is a common thing for kubernetes. That would also be a sane default value at the helm chart layer too.
| @@ -2,16 +2,30 @@ package v1alpha1_connectionservice | |||
|
|
|||
There was a problem hiding this comment.
Ok, it's going to be easier to talk holistically about this here as there are a few things that I think need to be revised.
- I don't think we can always assume that every
SqlConnectionthat comes out of K8s will contain a Secret. There could be some that use theValuedirectly, and the API should seamlessly support that. - I am seeing a few assumptions that the
SqlConnectionwill always be Postgres, which is of course the case now, but will quickly not be. It's really classified as a postgres connection if the driver specified says as such. - I'm having some thoughts about the UUID label. Now that I'm seeing the code, it's making me think that the presence of it makes this api more brittle. I am thinking this because I'm coming from the place where I have manually created a
JobConfigdirectly from yaml, and now just want to go into the app to see it. However, it won't work because it'll be filtered out!
3.1 I wonder if the right thing to do here is to actually have the operator attach the uuid to it as a label. That way we can be sure it is always there, regardless of how the JobConfig arrived. Thoughts? Should be relatively straightforward to do. Then the backend can just look for it on all of the CRDs that are owned by the operator.
3.1.1 The other option I think we talked about was to just use the name directly right? Hmm.
There was a problem hiding this comment.
I'm also wondering about the case where we want to return a list of Connections, but maybe one has an invalid secret. What should we do there? Fail hard? Or maybe filter that one out? Or possibly return it but have it be in an invalid state perhaps?
There was a problem hiding this comment.
- will fix
- will fix
- I think the operator should attach the uuid as a label. I prefer uuid over name
There was a problem hiding this comment.
I'm also wondering about the case where we want to return a list of Connections, but maybe one has an invalid secret. What should we do there? Fail hard? Or maybe filter that one out? Or possibly return it but have it be in an invalid state perhaps?
I think the best user experience is probably return but have it in invalid state. That way they can see the issue and fix. Otherwise who knows what the real issue is
Removes postgres and uses kubernetes for connection source