*: test the case when both app and CA TLS assets exist #439
Conversation
|
test are not being ran in the CI. ref: #440 |
|
@fanminshi Can move this test to And until @AlexNPavel refactors to add in a dynamic client to that framework you can still use the core k8s client exposed by the framework to CRUD the secrets/configmaps in this test. |
|
@hasbro17 yeah, I am planning to move this under test/e2e. Any reason that we can't our own dynamic client? |
I don't want to use a global client that's not explicitly part of the framework. Plus one more thing we have to refactor out when we move to the controller-runtime. Can you just use the regular core client for now. |
|
yeah, that's fine with me. |
fanminshi
changed the title
fanminshi
changed the title
fanminshi
changed the title
|
CI passed. PTAL cc/ @hasbro17 |
test/e2e/tls_util_test.go
Outdated
| } | ||
|
|
||
| certName := "app-cert" | ||
| appSecretName := strings.ToLower(crKind) + "-" + crName + "-" + certName |
There was a problem hiding this comment.
You can make the tls name utils public and use them over here so they're the same thing.
https://github.com/operator-framework/operator-sdk/blob/master/pkg/tlsutil/tls.go#L184
appSecretName := tlsutil.ToAppSecretName(crKind, crName, certName)
For this and caConfigMapAndSecretName below as well.
fanminshi
changed the title
| t.Fatal(err) | ||
| } | ||
|
|
||
| if !reflect.DeepEqual(appSecret, actualAppSecret) { |
There was a problem hiding this comment.
Can we avoid doing DeepEqual() for the entire objects. That's usually not very helpful as things like resourceVersion might change if the object is updated after creation. Or some label or annotation might be added by the server after creation.
The checks should be as specific as possible. Either check something in the spec to verify equality. Or do it for the UID which uniquely identifies the object to verify they're both the same objects.
if !reflect.DeepEqual(appSecret.UID, actualAppSecret.UID)or
if appSecret.UID != actualAppSecret.UIDsince they're just strings.
There was a problem hiding this comment.
That's usually not very helpful as things like resourceVersion might change if the object is updated after creation
This shouldn't happen because we don't update the secrets and configmap in this test case. I expect the created objects are the exact same as the one returned by GenerateCert().
There was a problem hiding this comment.
It's okay for this case but my general point is when either reconciling or checking a test condition we should be as specific as possible when checking the equality of two objects.
In this case even if say the resourceVersion changes (for whatever reason) our test should not fail because we just care if it's the same secret object that we created earlier. And that's verifiable by just looking at something unique(like the UID).
Although even the name/namespace uniquely identifies the secret so we don't even need this check strictly speaking.
But you can keep it as !reflect.DeepEqual(appSecret.UID, actualAppSecret.UID) for this case.
test/e2e/tls_util_test.go
Outdated
| } | ||
|
|
||
| if !reflect.DeepEqual(appSecret, actualAppSecret) { | ||
| t.Fatalf("expect %v, got %v", appSecret, actualAppSecret) |
There was a problem hiding this comment.
This will be pretty verbose.
Let's use %+v to get the field names to make it easier to read at least.
test/e2e/tls_util_test.go
Outdated
| t.Fatal(err) | ||
| } | ||
|
|
||
| // treat the Pod manifest as the input CRfor the GenerateCert(). |
There was a problem hiding this comment.
Maybe clarify this comment a bit more:
Use Pod as a dummy runtime object for the CR input of GenerateCert()
|
LGTM |
2 participants
Test the case when both app and ca tls assets exist in the cluster, the GenerateCert should just return those k8s assets to the user.
Also change SDKCertGenerator to use
kubernetes.Interfaceinstead of the sdk's dynamic client.