Further fluxv2 plugin features: misc fixes and add an integration test for CreateInstalledPackage #3390
Conversation
| // returns 3 things: | ||
| // - complete whether the operation was completed | ||
| // - success (only applicable when complete == true) whether the operation was successful or failed | ||
| // - reason, if present |
There was a problem hiding this comment.
Excellent, thanks for making that clear.
| @@ -212,16 +214,19 @@ func (s *Server) installedPackageDetail(ctx context.Context, name types.Namespac | |||
| return nil, status.Errorf(codes.NotFound, "Unable to find Helm release %q due to: %v", name, err) | |||
| } | |||
|
|
|||
| log.Infof("installedPackageDetail:\n[%s]", prettyPrintMap(unstructuredRelease.Object)) | |||
There was a problem hiding this comment.
Should that be a debug msg? Could be quite noisy if we're logging complete objects at info.
There was a problem hiding this comment.
absolutely. was a left-over
| // err maybe NotFound if this object has just been created and flux hasn't had time | ||
| // to invoke helm layer yet | ||
| if err == nil && release != nil { | ||
| // a couple of fields currrently only available via helm API | ||
| appVersion = release.Chart.AppVersion() | ||
| if release.Info != nil { | ||
| postInstallNotes = release.Info.Notes | ||
| } |
There was a problem hiding this comment.
Fine if you don't like them, but this is where you can use those Getters that we chatted about a while back, so that the above 4 lines can be just:
appVersion = release.GetChart().GetAppVersion()
postInstallNotes = release.GetInfo().GetNotes()
There was a problem hiding this comment.
release.GetChart undefined (type *"helm.sh/helm/v3/pkg/release".Release has no field or method GetChart)compilerMissingFieldOrMethod ???
| appVersion, postInstallNotes := "", "" | ||
| release, err := s.helmReleaseFromUnstructured(ctx, name, obj) | ||
| // err maybe NotFound if this object has just been created and flux hasn't had time | ||
| // to invoke helm layer yet |
There was a problem hiding this comment.
We still need to handle the error though right? We can check for a not found error and choose to ignore that specifically, but if it's any other error, it should be returned, shouldn't it?
There was a problem hiding this comment.
this is just for a couple of fields out of more than a dozen. It's not a "deal-breaker". I will log the error for now.
| // - userReason: textual description of why the object is in current state, if present | ||
| // docs: | ||
| // 1. https://fluxcd.io/docs/components/helm/helmreleases/#examples | ||
| // 2. discussion on https://vmware.slack.com/archives/C4HEXCX3N/p1630907107078800. |
There was a problem hiding this comment.
I don't think it's helpful for us to be linking to private discussions (the above link is for an internal thread).
Instead should be having those discussions on the public slack where appropriate, then we could link to them for more context if required.
There was a problem hiding this comment.
Ouch. I was just trying to be helpful for the next guy who might need some context to maintain the code. Anyway, your objection noted :-) What do you suggest I do in this case? Copy and paste the whole thread into the code from slack?
There was a problem hiding this comment.
I'd just mention/summarise what was important from the discussion, in this case.
There was a problem hiding this comment.
ok will do just that. Thanks again for the review
| } | ||
| if statusString, ok := conditionAsMap["status"]; ok { | ||
| if statusString == "True" { | ||
| return true, corev1.InstalledPackageStatus_STATUS_REASON_INSTALLED, userReason |
There was a problem hiding this comment.
It's confusing to me that isInstallFailed can (theoretically) be true at this point from above. Why don't you return earlier than here if isInstallFailed is true? (perhaps 3 lines up before checking the status?)
There was a problem hiding this comment.
not theoretical, real case. tested and reproducible, if needed. I thought about it, and I think my way is easier to read. status.condition.Ready == false and reason = "InstallFailed"
There was a problem hiding this comment.
just fyi, here is a real-life example from an integration test I am working on that shows this use case:
[{
"apiVersion": "helm.toolkit.fluxcd.io/v2beta1",
"kind": "HelmRelease",
"metadata": {
"creationTimestamp": "2021-09-14T03:27:41Z",
"finalizers": [
"finalizers.fluxcd.io"
],
"generation": 2,
"managedFields": [
{
"apiVersion": "helm.toolkit.fluxcd.io/v2beta1",
"fieldsType": "FieldsV1",
"fieldsV1": {
"f:spec": {
".": {},
"f:chart": {
".": {},
"f:spec": {
".": {},
"f:chart": {},
"f:sourceRef": {
".": {},
"f:kind": {},
"f:name": {},
"f:namespace": {}
},
"f:version": {}
}
},
"f:install": {
".": {},
"f:createNamespace": {}
},
"f:targetNamespace": {},
"f:values": {}
}
},
"manager": "kubeapps-apis",
"operation": "Update",
"time": "2021-09-14T03:27:41Z"
},
{
"apiVersion": "helm.toolkit.fluxcd.io/v2beta1",
"fieldsType": "FieldsV1",
"fieldsV1": {
"f:metadata": {
"f:finalizers": {
".": {},
"v:\"finalizers.fluxcd.io\"": {}
}
},
"f:spec": {
"f:interval": {}
},
"f:status": {
"f:conditions": {},
"f:failures": {},
"f:helmChart": {},
"f:installFailures": {},
"f:lastAttemptedRevision": {},
"f:lastAttemptedValuesChecksum": {},
"f:observedGeneration": {}
}
},
"manager": "helm-controller",
"operation": "Update",
"time": "2021-09-14T03:27:42Z"
}
],
"name": "my-podinfo-5",
"namespace": "kubeapps",
"resourceVersion": "1707794",
"uid": "9e574bd3-61bd-4b5f-9b46-33466c4b8773"
},
"spec": {
"chart": {
"spec": {
"chart": "podinfo",
"sourceRef": {
"kind": "HelmRepository",
"name": "podinfo-5",
"namespace": "default"
},
"version": "*"
}
},
"install": {
"createNamespace": true
},
"interval": "1m0s",
"targetNamespace": "test-5-mdnt",
"values": {
"replicaCount": "what we do in the shadows"
}
},
"status": {
"conditions": [
{
"lastTransitionTime": "2021-09-14T03:27:42Z",
"message": "install retries exhausted",
"reason": "InstallFailed",
"status": "False",
"type": "Ready"
},
{
"lastTransitionTime": "2021-09-14T03:27:42Z",
"message": "Helm install failed: unable to build kubernetes objects from release manifest: error validating \"\": error validating data: ValidationError(Deployment.spec.replicas): invalid type for io.k8s.api.apps.v1.DeploymentSpec.replicas: got \"string\", expected \"integer\"\n\nLast Helm logs:\n\n",
"reason": "InstallFailed",
"status": "False",
"type": "Released"
}
],
"failures": 12,
"helmChart": "default/kubeapps-my-podinfo-5",
"installFailures": 1,
"lastAttemptedRevision": "6.0.0",
"lastAttemptedValuesChecksum": "a619eeb520124dc343fb0222f3f7d0c27378a983",
"observedGeneration": 2
}
}]
There was a problem hiding this comment.
Yes, that case makes sense to me. My question was because it looked like above we already know that installFailed == true yet, we're on a conditional branch returning ready = true and installed successfully (i understand that condition may not be possible to get to true at that point).
| // at this point. | ||
| // 3) run ./kind-cluster-setup.sh once prior to these tests | ||
|
|
||
| // if (1) or (2) of the above pre-requisites is not satisfied, the tests are simply skipped |
There was a problem hiding this comment.
Thanks for adding the integration tests!
Hmm, if I intend for these integration tests to run, but the environment isn't correct (eg. forgot to port-forward), the tests are skipped? Would it be cleaner to run the integration tests if an env var is set and explicitly fail if the environment hasn't been setup correctly? This way, if I intend to run these, they'll either run or fail, rather than being skipped?
There was a problem hiding this comment.
yes on the first question. You mean like in postgress_db_tests.go:
func SkipIfNoDB(t *testing.T) {
if !dbutilstest.IsEnvVarTrue(t, EnvvarPostgresTests) {
t.Skipf("skipping postgres tests as %q not set to be true", EnvvarPostgresTests)
}
}
Yes, I could do that. You suggested just a few days ago not to invest too much energy into allowing others, including CI to run these, so i didn't do anything else. I can make this change, just confirm that's what you'd like. Update, I just made the change, hope its what you meant
| dynamicClient, _, err = s.clientGetter(context.Background()) | ||
| if err != nil { | ||
| t.Fatalf("%+v", err) | ||
| } |
There was a problem hiding this comment.
Excellent, thanks for adding the checks for the release CRD in the unit tests.
| t.Fatalf("%+v", err) | ||
| } | ||
|
|
||
| if got, want := releaseObj.Object, tc.expectedRelease; !cmp.Equal(want, got) { |
There was a problem hiding this comment.
Surprised you didn't need to ignore some fields... great, if so.
There was a problem hiding this comment.
yeah, me too. But it works, I tested it. Maybe these CRDs don't have any unexported fields?
No description provided.