reconcile pyxis api secret #23
Conversation
openshift-ci
bot
added
the
needs-rebase
skattoju
force-pushed
the
reconcile_pyxis_api_secret
branch
from
15ffac5
to
4d96ac7
Compare
skattoju
force-pushed
the
reconcile_pyxis_api_secret
branch
from
4d96ac7
to
dc64a99
Compare
openshift-ci
bot
added
needs-rebase
skattoju
force-pushed
the
reconcile_pyxis_api_secret
branch
from
d866b8f
to
7557dbd
Compare
openshift-ci
bot
removed
the
needs-rebase
controllers/secret.go
Outdated
| log.Error(err, fmt.Sprintf("unable to get the %s secret", secretName)) | ||
| return err | ||
| } | ||
| if kubeConfigSecret.Data[KUBECONFIG_SECRET] == nil { |
There was a problem hiding this comment.
This references the kubeconfig value. the value for this key should be
PYXIS_API_KEY = "pyxis_api_key"
Also. checking in a map this way can throw a panic if the key isn't present.
if value, ok := kubeConfigSecret.Data[KUBECONFIG_SECRET]; ok{
//then evaluate data in value
}
There was a problem hiding this comment.
I think if the requested key doesn’t exist, we get the value type’s zero value.. seems safe in golang ?
https://play.golang.org/p/rTtRDqMdgMa
There was a problem hiding this comment.
Missed that was the same as the secret name in the other two cases.. i think.
controllers/secret.go
Outdated
| log.Info("existing kubeconfig secret found") | ||
| return nil // Existing Secret found, do nothing... | ||
| if !IsObjectFound(r.Client, key, secret) { | ||
| err := errors.New(fmt.Sprintf("An existing secret named %s was not found in namespace %s", secretName, meta.Namespace)) |
There was a problem hiding this comment.
This seems to be a generic error that I have seen in other files too. Can we move this error definition to a file and reuse it everywhere.
There was a problem hiding this comment.
agreed, should be fixed in #24
controllers/secret.go
Outdated
| kubeConfigSecret := &corev1.Secret{} | ||
| err = r.Client.Get(context.TODO(), key, kubeConfigSecret) | ||
| if err != nil { | ||
| log.Error(err, fmt.Sprintf("unable to get the %s secret", secretName)) |
There was a problem hiding this comment.
It would be good to log the namespace as well.
There was a problem hiding this comment.
its logged right before as an info but I agree its better to include it..
have not looked into into completely but apparently the k8s runtime logging interface should be able add contextual info like namespace etc to the logs 🤔
controllers/secret.go
Outdated
| err = r.Client.Get(context.TODO(), key, kubeConfigSecret) | ||
| if err != nil { |
There was a problem hiding this comment.
These two lines could be merged into one
controllers/secret.go
Outdated
| log.Error(err, fmt.Sprintf("unable to get the %s secret", secretName)) | ||
| return err | ||
| } | ||
| if kubeConfigSecret.Data[KUBECONFIG_SECRET] == nil { |
There was a problem hiding this comment.
is there any chance that the data is not nil, but is empty? I believe we should check for the empty string as well.
There was a problem hiding this comment.
added an || len == 0
skattoju
force-pushed
the
reconcile_pyxis_api_secret
branch
from
7557dbd
to
d7ad759
Compare
Fixes: #9
The pyxis api secret name can optionally be configured via the cr
This is going to have a bunch of repeated code to be addressed in #24