feat: Implement the GetHelmRelease endpoint #1534

yitsushi · 2022-02-25T15:39:03Z

Add endpoint to get a single HelmRelease.

Additional changes:

Moved ListHelmReleases and GetHelmRelease endpoint definitions into
their own file, to get a better separation (helm.go).
Moved the tests for ListHelmReleases and GetHelmRelease endpoints into
the corresponding test file (helm_test.go).

For Kustomization, the getInventory function is under types, but to
get the inventory for a HelmRelease, we need a lot of extra queries from
k8s and we already have a lot of helper functions in server.

Refactored tests to use server_test module instead of server.

Benefits:

If a branch is not covered with test, it might be deleted, or some
cases are not tested.
We test the same interface as a consumer will use.
We can't "reuse" private functions for testing, that way we can see
during testing if we have to publish a function or not.

Closes #1498

Add endpoint to get a single HelmRelease. Additional changes: * Moved `ListHelmReleases` and `GetHelmRelease` endpoint definitions into their own file, to get a better separation (`helm.go`). * Moved the tests for `ListHelmReleases` and `GetHelmRelease` endpoints into the corresponding test file (`helm_test.go`). Closes #1498

jpellizzari

@yitsushi I would have expected to see some work in types/helmrelease.go to get the inventory stuff working. Is that already taken care of?

jpellizzari · 2022-02-25T16:10:14Z

core/server/automations.go

@@ -57,25 +56,3 @@ func (cs *coreServer) GetKustomization(ctx context.Context, msg *pb.GetKustomiza

 	return &pb.GetKustomizationResponse{Kustomization: res}, nil
 }
-
-func (cs *coreServer) ListHelmReleases(ctx context.Context, msg *pb.ListHelmReleasesRequest) (*pb.ListHelmReleasesResponse, error) {


Moving this to a new file makes the naming inconsistent. If we are really going for that level of separation, we also need to rename automations to kustomizations, since that is all it contains now.

I would like to split them up, later on much easier to find and change without worrying about a conflict between PRs. I'll rename the file to helm_release and automations to kustomization

For Kustomization, the getInventory function is under `types`, but to get the inventory for a `HelmRelease`, we need a lot of extra queries from k8s and we already have a lot of helper functions in `server`.

yitsushi · 2022-02-25T18:45:20Z

I would be happy to test the inventory part, but I have no idea how to even approach that part, with creating a secret, and create encoded stuff with that secret in there. If required, I'll spend more time and figure it out.

Callisto13 · 2022-02-28T12:13:43Z

core/server/helm_release.go

+		entry.GetAPIVersion()
+
+		idstr := strings.Join([]string{entry.GetAPIVersion(), entry.GetKind()}, "_")


Do you know why we call entry.GetAPIVersion here and then again on 134? I can see we are doing it somewhere else in the code, so am curious what repeating but not keeping the value gives us.

I have no idea, and because I have no idea, I just used the same logic XD

does it hurt anything if you remove the first one?

while testing it does not hurt, but I don't know if it does with real k8s objects coming from a real k8s cluster.

ack, makes sense, would be good to prove and remove at somepoint

Removed... Why?

entry.GetAPIVersion() idstr := strings.Join([]string{entry.GetAPIVersion(), entry.GetKind()}, "_")

The function call sequence is:

entry.GetAPIVersion()

entry.GetKind()

strings.Join()

With that extra call the difference is an extra call only:

entry.GetAPIVersion()

entry.GetAPIVersion()

entry.GetKind()

strings.Join()

I see only one case when it can have any effects, if the underlying code is far from optimal and has an internal counter (how many times it was called) or something, and it does not make sense.

Callisto13 · 2022-02-28T12:15:27Z

core/server/helm_release.go

+
+	var gvk []*pb.GroupVersionKind
+
+	found := map[string]bool{}


we could probs use map[string]struct{} here if we care about speed and memory, but bool reads better so happy either way.

Callisto13 · 2022-02-28T12:18:12Z

core/server/helm_release_test.go

@@ -0,0 +1,109 @@
+package server


can we make this server_test? we are testing an exposed interface, so we can test against it from the outside

I'm 100% on the that side (blackbox testing), but the rest of the code follows this logic (whitebox testing), so I did the same.

yeh. i would be good to move over while we are starting new so we don;t blur lines

Did a few pokes on it, all the tests are using the same functions (and global variable 😞 ), so I moved everything and now it's all server_test

Callisto13 · 2022-02-28T12:20:09Z

core/server/helm_release_test.go

+	g.Expect(res.HelmReleases[0].Name).To(Equal(appName))
+}
+
+func TestGetHelmRelease(t *testing.T) {


are you going to test any of the getHelmReleaseInventory bits? it feels important to us that we are populating that bit of the returned value correct

ah just literally saw your comment on that 👍

@yitsushi Here is how I had to test statuses:

weave-gitops/core/server/automations_test.go

Line 110 in 0fc37a2

g.Expect(k.Status().Patch(ctx, kust, client.Apply, opt...)).To(Succeed())

Note that this is the second .Patch, after creating the record initially with the first .Patch (instead of .Create).

My issue with testing that function... well I have to create a full secret + store + data + "i don't know what else", because unlike kustomization, to get helm inventory, we query the api to get a helm secret, get release from the secret storage, decode + unzip (if gzipped), and finally read object from the storage. or something like that... so that would require a huge bootstrapping. I'll try it, because I would like to test it.

black-box testing environment and we test what the consumer will use. Benefits: * If a branch is not covered with test, it might be deleted, or some cases are not tested. * We test the same interface as a consumer will use. * We can't "reuse" private functions for testing, that way we can see during testing if we have to publish a function or not.

jpellizzari

LGTM 👍. Up to you if you want to add more test coverage around Invnentory.

jpellizzari · 2022-02-28T15:53:04Z

core/server/fluxruntime_test.go

@@ -1,11 +1,12 @@
-package server
+package server_test


This is the opposite of how our team has been structuring packages to this point. I don't really care either way, but we need to decide as an eng org how we want to do this.

It's probably a per-codebase decision. It's not the end of the world to white box test some layers, but it is not great practice when it comes to external facing contracts (which these are). Given we are tidying things, now feels like a good time to clean this up.

jpellizzari · 2022-02-28T15:59:24Z

core/server/helm_release_test.go

+	g.Expect(res.HelmReleases[0].Name).To(Equal(appName))
+}
+
+func TestGetHelmRelease(t *testing.T) {


@yitsushi Here is how I had to test statuses:

weave-gitops/core/server/automations_test.go

Line 110 in 0fc37a2

g.Expect(k.Status().Patch(ctx, kust, client.Apply, opt...)).To(Succeed())

Note that this is the second .Patch, after creating the record initially with the first .Patch (instead of .Create).

yitsushi force-pushed the 1498-get-helm-release-endpoint branch from 88af153 to 3df82c4 Compare February 25, 2022 15:40

yitsushi requested a review from jpellizzari February 25, 2022 15:57

jpellizzari reviewed Feb 25, 2022

View reviewed changes

Add HelmRelease inventory

f202d07

For Kustomization, the getInventory function is under `types`, but to get the inventory for a `HelmRelease`, we need a lot of extra queries from k8s and we already have a lot of helper functions in `server`.

yitsushi requested a review from jpellizzari February 25, 2022 18:39

Callisto13 reviewed Feb 28, 2022

View reviewed changes

yitsushi added 2 commits February 28, 2022 15:21

Remove unnecessary GetAPIVersion call

f63e2d5

jpellizzari approved these changes Feb 28, 2022

View reviewed changes

yitsushi merged commit 657e092 into v2 Mar 2, 2022

yitsushi mentioned this pull request Mar 2, 2022

chore(test): Add inventory tests on GetHelmRelease #1566

Merged

ozamosi deleted the 1498-get-helm-release-endpoint branch May 12, 2022 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement the GetHelmRelease endpoint #1534

feat: Implement the GetHelmRelease endpoint #1534

yitsushi commented Feb 25, 2022 •

edited

jpellizzari left a comment

jpellizzari Feb 25, 2022

yitsushi Feb 25, 2022

yitsushi commented Feb 25, 2022

Callisto13 Feb 28, 2022

yitsushi Feb 28, 2022

Callisto13 Feb 28, 2022

yitsushi Feb 28, 2022

Callisto13 Feb 28, 2022

yitsushi Feb 28, 2022

Callisto13 Feb 28, 2022

Callisto13 Feb 28, 2022

yitsushi Feb 28, 2022

Callisto13 Feb 28, 2022

yitsushi Feb 28, 2022 •

edited

Callisto13 Feb 28, 2022

Callisto13 Feb 28, 2022

jpellizzari Feb 28, 2022

yitsushi Feb 28, 2022 •

edited

jpellizzari left a comment •

edited

jpellizzari Feb 28, 2022

Callisto13 Feb 28, 2022

jpellizzari Feb 28, 2022

		entry.GetAPIVersion()

		idstr := strings.Join([]string{entry.GetAPIVersion(), entry.GetKind()}, "_")

feat: Implement the GetHelmRelease endpoint #1534

feat: Implement the GetHelmRelease endpoint #1534

Conversation

yitsushi commented Feb 25, 2022 • edited

jpellizzari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitsushi commented Feb 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitsushi Feb 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitsushi Feb 28, 2022 • edited

Choose a reason for hiding this comment

jpellizzari left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yitsushi commented Feb 25, 2022 •

edited

yitsushi Feb 28, 2022 •

edited

yitsushi Feb 28, 2022 •

edited

jpellizzari left a comment •

edited