Make it easy to find out the k8s and flux version #2349

opudrovs · 2022-06-21T16:55:57Z

Closes #2285
Closes #2328

I've added a namespace to the GetVersionRequest object to make TestGetVersion pass because it could not find the flux-system namespace without it. So, for the test I create a new namespace or if the namespace is not provided default to flux-system (wego.DefaultNamespace constant). If anyone knows how to improve this and the issue with nested ifs (will add a comment), I'm all ears. 🙂

UPDATE: Removed the namespace field from GetVersionRequest (switched to listing namespaces) and fixed nested ifs based on @ozamosi 's suggestions.

I've copy-pasted the test for GetImpersonatedDiscoveryClient from the test for GetImpersonatedClient. I am not sure what this API does and what exactly should be tested, maybe it should be modified.

Also removed the following from the copied test, maybe it should be put back (this part was failing because the number of found namespaces in the copy-pasted part was 0 instead of 1 or 2):

t.Run("checks all namespaces in the cluster when through the filtering", func(t *testing.T) {
	g.Expect(nsChecker.FilterAccessibleNamespacesCallCount()).To(Equal(1))

	_, _, nss := nsChecker.FilterAccessibleNamespacesArgsForCall(0)
	nsFound := 0
	for _, n := range nss {
		if n.Name == ns1.Name || n.Name == ns2.Name {
			nsFound++
		}
	}

	g.Expect(nsFound).To(Equal(2))
})

opudrovs · 2022-06-24T12:48:25Z

core/server/version.go

+				fluxVersion = u.GetLabels()["app.kubernetes.io/version"]
+			}
+		}
+	}


@ozamosi @chanwit suggestions on how to simplify nested ifs here (looks similar to a "callback hell" in JS) welcome!

I don't want to return nil from GetVersion on error when getting additional values, because I want to display at least available values (at least the values set at the build time) in the app for debug purposes.

Maybe lift this bit into a new function, return an error from there any time there's an error, and then this function only has one error to check?

Thanks, good idea, will do it.

opudrovs · 2022-06-24T13:16:32Z

@ozamosi @chanwit also, maybe I can use the VersionLabelKey from the flux.go instead of app.kubernetes.io/version to get the label value?

ozamosi · 2022-06-24T13:43:06Z

core/server/version.go

+
+	key := client.ObjectKey{
+		Name: ns,
+	}


So, you're right that this can be pretty much any namespace - but I'm not sure the frontend knows what namespace to use 🤔

What if instead of Geting a specific namespace, we List filtered by {"app.kuberenetes.io/part-of": "flux"} and just grab the first result we find? After a bit of grepping - I think this helper does the filtering? https://github.com/weaveworks/weave-gitops/blob/main/core/server/server.go#L86

OK, can do it.

From frontend we don't provide any namespace. This param was added just for passing the test (because when running the test it could not find the "flux-system" namespace). If the namespace is not provided (like when we make the request via the API, {} with no args), we just default to "flux-system" (and while the app is running normally, the "flux-system" namespace is found). At least that was the idea. 🙂

I need to see if the list function will return the flux-system/flux-whatever namespace while running the test.

ozamosi · 2022-06-24T15:38:41Z

@ozamosi @chanwit also, maybe I can use the VersionLabelKey from the flux.go instead of app.kubernetes.io/version to get the label value?

Sounds good!

chanwit · 2022-06-24T15:50:35Z

core/server/version.go

+	if err != nil {
+		return defaultVersion, fmt.Errorf("error getting object: %w", err)
+	} else {
+		return u.GetLabels()["app.kubernetes.io/version"], nil


u.GetLabels() may return nil here.

never mind me @opudrovs, if you chose to use the VersionLabelKey instead.

Thanks, will keep it in mind.

opudrovs · 2022-06-27T08:41:40Z

@ozamosi @chanwit added grabbing a namespace from the list of namespaces. For testing, I create a new namespace with required key because the flux-ns namespace available during testing doesn't have a Flux version.

Also moved creating scoped client, getting Flux and K8s version to separate functions, based on @ozamosi 's suggestion. Should the create scoped client function's name start with create or get, preferably?

@chanwit just in case added checking for nil when getting the version from labels.

@joshri I've added displaying Flux and Kubernetes versions in the footer. Also added noWrap to the Text component and removed neutral30 color specified for some footer elements because it is already specified for the whole footer, it looks like, the elements already inherit this color from footer. If it is wrong, can move it back.

Spaced and colored elements based on the quick design in the UI ticket description to make it consistent with current styling of the Footer component. Please let me know if styling/spacing/whatever is wrong.

opudrovs · 2022-06-27T09:11:09Z

@ozamosi @chanwit 1) would it be possible to filter for Version label key with any value? Or only filtering by specific values is possible?

opts := client.MatchingLabels{
    coretypes.PartOfLabel: FluxNamespacePartOf,
    flux.VersionLabelKey: <any value>
}

Right now I am filtering labels only by coretypes.PartOfLabel: FluxNamespacePartOf, and then loop through the result and find a label which has flux.VersionLabelKey key.

Is it OK to use functions result if I am also returning an error for it (in case of an error, I am returning the default version from flux and kube version functions, use the returned version as expected, and just log the error).

ozamosi · 2022-06-27T12:07:40Z

Should the create scoped client function's name start with create or get, preferably?

I feel Get feels more natural, since when calling the function I don't care if it's allocating a new client or reusing a cached one.

would it be possible to filter for Version label key with any value?

I've not seen that, and a quick peek doesn't reveal anything. So, maybe not?

Is it OK to use functions result if I am also returning an error for it

It's uncommon.

I prefer to think of it as, go has exceptions, it's just that they don't bubble up automatically so you have to constantly wire them up manually. So every time I see res, err := Function(); if err != nil { return err } it's just line noise that does what you get for free by calling throw Exception() in another language. As a result, if you do something that you couldn't express with "return value, or throw exception", then I have to go and review the code more carefully.

That doesn't mean it's always wrong. But in this case, if the returned value is simply an empty string, then I'd write it so that the calling function sets the default instead of the called function - so the fall-back is implemented as an "exception handler".

opudrovs · 2022-06-27T12:10:38Z

@ozamosi OK, thank you, got it, will add the changes now.

opudrovs · 2022-06-27T12:41:57Z

@ozamosi added the fixes.

joshri

I'm getting this back right now on the version request so nothing shows up in the page: branch: "" buildTime: "" commit: "" semver: "v0.0.0"
We might want to add a dash or something if no data comes through but the request was successful?

joshri · 2022-06-27T16:28:13Z

ui/components/Footer.tsx

+    ) : null;
+
+  const kubeVersionText = versionData.kubeVersion;
+  const kubeVersion =


fluxVersion and kubeVersion could potentially be combined into one component with props but I don't mind either way

Good idea, will do it.

Added the Version component with snapshot tests.

Not sure if adding attrs when exported is still required, like recommended in the style.md doc, so still added it like that:

export default styled(Version).attrs({ className: Version.name })``;

Footer versions are wrapped at narrow screens (see layout at 768 or 1000 px for example) , otherwise we would have to add a horiz scrollbar, preferably for the whole layout (or it looks ugly if added only to the footer).

Does this look OK?

Three options that come to mind:

Set everything to nowrap and just have it overflow the container

Increase the minWidth of the container so we know we're always fitting the whole footer in

Decrease font size on the footer?
Maybe we do a mix of all three?

Used options 1 and 2: set the Version component text to nowrap and changed 768px in Layout.tsx from to 900px (conveniently, a Material-UI breakpoints).

Everything I tried with scrollbars in the footer and footer containers looked ugly. So, I've decided not to add overflow-x: auto.

I think it's either these two applied fixes or we'd need to do a serious redesign of the footer (for example, move it under the whole app content or move content info + email text somewhere out of the footer), because the current footer text is really wide.

So, the current version looks best for now.

joshri · 2022-06-27T16:28:37Z

ui/components/Footer.tsx

  return (
    <Flex as="footer" wide between className={className} role="footer">
      <LeftFoot>
-        <Text color="neutral30">Need help? Contact us at</Text>
+        <Text>Need help? Contact us at</Text>


add the excellent noWrap prop here

joshri · 2022-06-27T16:29:00Z

ui/components/Footer.tsx

        )}
        <Spacer padding="xxs" />
-        <Text color="neutral30">© 2022 Weaveworks</Text>
+        <Text>© 2022 Weaveworks</Text>


same as above - add the excellent noWrap prop!!

opudrovs · 2022-06-27T20:09:18Z

@joshri

I'm getting this back right now on the version request so nothing shows up in the page: branch: "" buildTime: "" commit: "" semver: "v0.0.0"

Did you open the app through Tilt or npm start? And are you sure you rebuilt everything and reloaded the app? Can you please try restarting Tilt?

We might want to add a dash or something if no data comes through but the request was successful?

Not a problem. Can add displaying the labels with dashes even if the Flux version and Kubernetes version are empty strings for any reason.

But there should be fields fluxVersion and kubeVersion in the network request response in any case.

joshri

I had to update something...classic. Rebuilt and the footer looks good!

Run `make proto`.

Add a discovery client to get the KUbernetes version. Fix error messages.

Add `Namespace` field to the `GetVersionRequest`.

Move creating scoped client to a separate function. Rework error handling. Fix getting the Flux version. Fix tests.

Rework error handling in `getFluxVersion` and `getKubeVersion` methods.

Update snapshot tests.

Increase min content width to 900px.

opudrovs added type/enhancement New feature or request team/denim labels Jun 21, 2022

opudrovs force-pushed the 2285-make-it-easy-to-find-out-k8s-and-flux-version branch 10 times, most recently from ebe2afe to f81f64f Compare June 23, 2022 13:58

opudrovs commented Jun 24, 2022

View reviewed changes

opudrovs marked this pull request as ready for review June 24, 2022 12:48

opudrovs requested review from chanwit and ozamosi June 24, 2022 12:48

ozamosi reviewed Jun 24, 2022

View reviewed changes

chanwit reviewed Jun 24, 2022

View reviewed changes

opudrovs force-pushed the 2285-make-it-easy-to-find-out-k8s-and-flux-version branch 2 times, most recently from 5c635c2 to e59fb72 Compare June 27, 2022 08:33

opudrovs requested a review from joshri June 27, 2022 08:35

opudrovs requested review from ozamosi and chanwit June 27, 2022 08:41

opudrovs force-pushed the 2285-make-it-easy-to-find-out-k8s-and-flux-version branch from f7249b3 to 871c8e2 Compare June 27, 2022 09:04

opudrovs force-pushed the 2285-make-it-easy-to-find-out-k8s-and-flux-version branch 2 times, most recently from 155cb5e to ee45d46 Compare June 27, 2022 09:18

opudrovs force-pushed the 2285-make-it-easy-to-find-out-k8s-and-flux-version branch from fdfb6a5 to 8d7324c Compare June 27, 2022 09:22

opudrovs force-pushed the 2285-make-it-easy-to-find-out-k8s-and-flux-version branch from a56b68e to 4766770 Compare June 27, 2022 15:08

joshri reviewed Jun 27, 2022

View reviewed changes

joshri approved these changes Jun 27, 2022

View reviewed changes

ozamosi approved these changes Jun 28, 2022

View reviewed changes

opudrovs force-pushed the 2285-make-it-easy-to-find-out-k8s-and-flux-version branch 2 times, most recently from 095f169 to 51d864d Compare June 28, 2022 14:25

opudrovs added 13 commits June 29, 2022 18:17

Add flux version and k8s version templates to the version endpoint.

492810b

Run `make proto`.

Add a scoped client to get the Flux version.

4002e65

Add GetImpersonatedDiscoveryClient method to ClientsFactory.

635f457

Add a discovery client to get the KUbernetes version. Fix error messages.

Fix the GetVersion test.

467565a

Add `Namespace` field to the `GetVersionRequest`.

Add getting Flux and Kubernetes version numbers.

aa78482

Move getting Flux and Kubernetes versions to separate functions.

72b677c

Change case of function names.

0151267

Grab the first suitable namespace from a list of namespaces.

90dd213

Move creating scoped client to a separate function. Rework error handling. Fix getting the Flux version. Fix tests.

Add displaying Flux and Kubernetes versions in the footer.

20745d1

Rename createScopedClient method to getScopedClient.

19c24bf

Rework error handling in `getFluxVersion` and `getKubeVersion` methods.

Add noWrap prop to text.

4364822

Add Version component.

53436c6

Update snapshot tests.

Prevent wrapping of version text in the footer.

46bf467

Increase min content width to 900px.

opudrovs force-pushed the 2285-make-it-easy-to-find-out-k8s-and-flux-version branch from 11fb4d3 to 46bf467 Compare June 29, 2022 16:17

opudrovs merged commit 93cd101 into main Jun 29, 2022

opudrovs deleted the 2285-make-it-easy-to-find-out-k8s-and-flux-version branch June 29, 2022 16:24

This was referenced Jul 26, 2022

Updates for 0.9.1-rc.2 #2491

Merged

Updates for 0.9.1 #2500

Merged

weave-gitops-bot mentioned this pull request Aug 10, 2022

Updates for 0.9.2-rc.1 #2578

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it easy to find out the k8s and flux version #2349

Make it easy to find out the k8s and flux version #2349

opudrovs commented Jun 21, 2022 •

edited

opudrovs Jun 24, 2022

ozamosi Jun 24, 2022

opudrovs Jun 24, 2022

opudrovs commented Jun 24, 2022

ozamosi Jun 24, 2022

opudrovs Jun 24, 2022 •

edited

ozamosi commented Jun 24, 2022

chanwit Jun 24, 2022

chanwit Jun 24, 2022

opudrovs Jun 24, 2022

opudrovs commented Jun 27, 2022 •

edited

opudrovs commented Jun 27, 2022 •

edited

ozamosi commented Jun 27, 2022

opudrovs commented Jun 27, 2022

opudrovs commented Jun 27, 2022

joshri left a comment

joshri Jun 27, 2022

opudrovs Jun 27, 2022

opudrovs Jun 28, 2022

joshri Jun 28, 2022

opudrovs Jun 28, 2022 •

edited

joshri Jun 27, 2022

opudrovs Jun 27, 2022

joshri Jun 27, 2022

opudrovs Jun 27, 2022

opudrovs commented Jun 27, 2022

joshri left a comment

Make it easy to find out the k8s and flux version #2349

Make it easy to find out the k8s and flux version #2349

Conversation

opudrovs commented Jun 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opudrovs commented Jun 24, 2022

Choose a reason for hiding this comment

opudrovs Jun 24, 2022 • edited

Choose a reason for hiding this comment

ozamosi commented Jun 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opudrovs commented Jun 27, 2022 • edited

opudrovs commented Jun 27, 2022 • edited

ozamosi commented Jun 27, 2022

opudrovs commented Jun 27, 2022

opudrovs commented Jun 27, 2022

joshri left a 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

opudrovs Jun 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

Choose a reason for hiding this comment

opudrovs commented Jun 27, 2022

joshri left a comment

Choose a reason for hiding this comment

opudrovs commented Jun 21, 2022 •

edited

opudrovs Jun 24, 2022 •

edited

opudrovs commented Jun 27, 2022 •

edited

opudrovs commented Jun 27, 2022 •

edited

opudrovs Jun 28, 2022 •

edited