-
Notifications
You must be signed in to change notification settings - Fork 706
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
fix for httpclient library SetClientTLS cleanup + unit tests #4119 & finish flux plugin: switch to using flux native data types instead of unstructured #4112 #4238
Conversation
…anzu#4119 plus a couple unrelated unit test clean up items in flux plugin
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.
Excellent, thanks Greg.
// TODO (gfichtenholt) | ||
// 1) we need to make sure that .List() always returns results in the | ||
// same order, otherwise pagination is broken (duplicates maybe returned and some entries | ||
// missing). |
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.
I'm still not clear on this: isn't your point (1) fixed by point (2)? That is, AIUI, if we update to pass the token used for pagination from the k8s api server, then we shouldn't be getting any duplicates or missing entries even when there is churn on the data level (ie. new HelmReleases or deleted ones).
Ah, or is the issue that the plugin is currently fetching all the objects and then returning just a subset for the paginated results of our server (in which case, we won't have the snapshot consistency, since each request to the kubeapps API will be getting results from a different snapshot?
func TestGetInstalledPackageSummariesWithPagination(t *testing.T) { | ||
// one big test case that can't really be broken down to smaller cases because | ||
// the tests aren't independent/idempotent: there is state that needs to be | ||
// kept track from one call to the next |
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.
Yep, makes sense, and the test itself is nice and clear.
t.Fatal(err) | ||
} else if got, want := payload, expectedPayload; !cmp.Equal(got, want) { | ||
t.Errorf("mismatch (-want +got):\n%s", cmp.Diff(want, got)) | ||
} |
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.
Nice, testing the actual server configured with the TLS.
plus a couple of un-related minor TODO clean up items in flux plugin unit tests leftover from previous PRs.
Update: had some extra time while CI was being repaired so I also finished the remainder of flux plugin: switch to using flux native data types instead of unstructured #4112