prometheus-operator: Add external_url #964

surajssd · 2020-09-14T14:24:59Z

This commit adds the feature removed sometime back called external_url
this allows the user to choose where(URL) the prometheus is exposed.

Fixes #826

TODOS:

Add test to verify the conversion.

invidian

OK

pkg/components/prometheus-operator/component_conversion_test.go

surajssd · 2020-09-22T11:26:25Z

@invidian PTAL at the latest commit, I have tried to make the test generic using some reflect magic.

This way is better to test conversion because we don't have to change the tests on every update. The Prometheus object has image tags and other values that might change on update.

invidian

Thanks for your effort on that @surajssd. I think this testing code doesn't look that bad from implementation point of view, even tough it uses reflect, so seems a bit like magic, but test definitions looks really crispy now, I really like them (except reflect.ValueOf bit I mentioned).

Perhaps @johananl could also have a look into it.

pkg/components/prometheus-operator/component_conversion_test.go

surajssd · 2020-10-05T07:58:56Z

@johananl @invidian PTAL

pkg/components/util/test/doc.go

pkg/components/util/test/jsonpath.go

pkg/components/internal/jsonpath.go

invidian

Some more thoughts/suggestions.

pkg/components/prometheus-operator/component.go

pkg/components/internal/common.go

invidian · 2020-10-08T08:50:04Z

pkg/components/internal/jsonpath.go

+		// TODO: Add type switch here for concrete types.
+		return got.Interface()
+	default:
+		t.Fatalf("Unknown type of the object extracted from the converted YAML: %v", got.Kind())


Does this ever occur?

I don't know yet. Since our tests are not that expansive. But it is best to capture it here than someone wasting time later debugging as to why some json paths won't work.

invidian · 2020-10-08T08:50:09Z

pkg/components/internal/jsonpath.go

+// getJSONPathValue extracts an object at a JSON path from a YAML config, and returns interface
+// object.
+func getJSONPathValue(t *testing.T, yamlConfig string, jsonPath string) interface{} {
+	u := getUnstructredObj(t, yamlConfig)
+	got := getValFromObject(t, jsonPath, u)
+
+	switch got.Kind() { //nolint:exhaustive
+	case reflect.Interface:
+		// TODO: Add type switch here for concrete types.
+		return got.Interface()
+	default:
+		t.Fatalf("Unknown type of the object extracted from the converted YAML: %v", got.Kind())
+	}
+
+	return nil
+}
+
+// MatchJSONPathStringValue is a helper function for component's unit tests. It compares a string
+// value of a JSON path in a YAML config.
+func MatchJSONPathStringValue(t *testing.T, yamlConfig string, jsonPath string, expected string) {
+	obj := getJSONPathValue(t, yamlConfig, jsonPath)
+
+	got, ok := obj.(string)
+	if !ok {
+		t.Fatalf("Value is not string: %#v", obj)
+	}
+
+	if got != expected {
+		t.Fatalf("Expected: %s, Got: %s", expected, got)
+	}
+}


I really have a feeling that using a struct here would make it more extensible (with basic types, not with reflect.Value) and make code simpler, because then switch got.Kind() { can just compare with ExpectedString field if found value is string and fail the test otherwise.

With reflect.Value or with the struct we will have a switch eventually. As we start supporting more values we will end up with it, its just matter of when.

invidian · 2020-10-08T08:56:59Z

pkg/components/prometheus-operator/component_test.go

+			component := newComponent()
+			m := testutil.RenderManifests(t, component, name, tc.inputConfig)
+			gotConfig := testutil.ConfigFromMap(t, m, tc.expectedManifestName)
+
+			testutil.MatchJSONPathStringValue(t, gotConfig, tc.jsonPath, tc.expected)


We could have one more helper to avoid copy-pasting those lines across components. The only changing bit here is component, though we need to make sure it's the "fresh" one for every test case.

It will be fresh one because we are calling newComponent unlike some tests where we extract the component object from the global map of components.

About putting it into a separate helper to avoid copying these three lines, I think lets wait until there are multiple of such places where we see it becoming very painful to maintain.

The idea is to re-use this code for multiple components from the very beginning. I don't understand why we can't make it easy to use for other components from the first version. It seems to me just like deferring this work to the person adding tests for other components...

Well, the same argument is true for not using reflect.Value. Why can't we use it right now it is like deferring the work to next person adding tests.

Also the problem with adding a helper here is that now we just don't have to pass component, but we also need to pass the name, the expected manifest name, json path and expected value. We can solve it by using struct like you mentioned before, dump everything in the struct and send that. But like I said we decided to revisit the struct vs reflect.Value later so I will defer this as well.

pkg/components/prometheus-operator/component_test.go

johananl

Thanks for the PR @surajssd.

My main concern is around moving a lot of the unit test logic to a separate package (which has "util" in its name...). I think that a prime concern when writing code is how easy it is to understand the code. It's a trade-off: On one hand, moving code to a central place introduces more "jumping around" when reading the code. On the other hand, excessive repetition within a source file can also make it hard to read and as a result - hard to understand.

In this case I am not convinced that we actually need to move any code to a new package. I feel that right now the principle we are following is "eliminate duplication at all costs" whereas I believe a better principle is "make code easy to understand at (nearly) all costs" 🙂 When faced with a choice between adding duplication and improving clarity, my bias is towards clarity. As a side note, unit tests specifically are an area in Go code which tends to be repetitive. I think this is perfectly fine because you want unit tests to be self-contained.

I don't want to block the PR on this and I would have to spend extra time to come up with an alternative, but I am worried that we are introducing a new pattern here (managing most of our test logic under testutil) which will be replicated from now on. I'll leave it to your good judgement, but please keep in mind that we may be introducing a new form of technical debt here.

Added a few additional comments inline, mainly around language.

docs/configuration-reference/components/prometheus-operator.md

pkg/components/prometheus-operator/component.go

pkg/components/internal/testutil/doc.go

pkg/components/internal/testutil/common.go

johananl · 2020-10-12T15:47:23Z

pkg/components/internal/testutil/common.go

+func ConfigFromMap(t *testing.T, m map[string]string, k string) string {
+	ret, ok := m[k]
+	if !ok {
+		t.Fatalf("Config not found with filename: %q", k)


I think we shouldn't call t.Fatalf from a helper function. The caller has to look at the implementation to figure out we're doing this, and the result could be that the helper function decides to terminate all the unit tests (which is what Fatalf does). IMO this is taking too much responsibility. I think we should consider returning an error and letting the caller decide what to do.

This applies for all other similar cases in pkg/components/internal/testutil.

The caller has to look at the implementation to figure out we're doing this, and the result could be that the helper function decides to terminate all the unit tests (which is what Fatalf does).

Fatalf only terminates single unit test. See https://golang.org/pkg/testing/#T.FailNow for more details.

Errorf can be used if you have data ready and you perform N checks on it, then each check may use Errorf. And we use Fatal here to break the execution flow, as otherwise we would test on bad data. IMO Fatalf use here is OK.

To stop the sub-test from proceeding I have used Fatalf. About returning things around in a test code makes no sense because we are already passing the test object (just like context) which can control the termination of the test.

I still think a helper function in package B shouldn't terminate a test in package A, but I don't want to block on this. I think we will likely realize we want to refactor this behavior in the future.

pkg/components/internal/testutil/jsonpath.go

johananl · 2020-10-12T16:02:56Z

pkg/components/internal/testutil/jsonpath.go

+	got, ok := obj.(string)
+	if !ok {
+		t.Fatalf("Value is not string: %#v", obj)
+	}
+
+	if got != expected {
+		t.Fatalf("Expected: %s, Got: %s", expected, got)
+	}


I am strongly against extracting this type of checks to a helper function. In my opinion these checks are the thing we want to include in each individual unit test, even if it makes things a bit repetitive. The reader should be able to figure out what a unit tests does without jumping around the codebase.

Right now it looks simplistic because we are just comparing strings. We would also add support for strings, numbers, arrays of concrete types then it will become complicated. So I would like to keep it here. So it becomes easy to extend.

IMO we shouldn't refactor things now because things may change in the future. We will be a lot smarter regarding how to refactor and simplify things when we actually see the problem. If the reader has to jump to a different source file when reading a unit test to understand what the test does, I think it's not ideal.

pkg/components/prometheus-operator/component.go

johananl · 2020-10-12T16:32:29Z

Forgot to say that I wasn't sure how to test the external_url knob. I can see that its value gets rendered in the YAML and the deployment seems to work fine, however I wasn't sure how to check the final result. For the future, I could definitely use some "how to test" instructions in the description 🙂

This commit adds the feature removed sometime back called `external_url` this allows the user to choose where(URL) the prometheus is exposed. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

There is still few threads opened which I would like to get resolved/fixed, but on the other hand I don't want to block merging this PR, so if others are fine with it, we can merge it and maybe we can re-visit it next time someone touches this code.

pkg/components/internal/testutil/common.go

johananl

Good to merge as far as I'm concerned (after fixing the typo).
I still don't like the approach we've taken regarding managing unit tests as I'm convinced we are reducing duplication at the cost of reducing readability and clarity. Let's leave it at that and consider refactoring if this code becomes hard to maintain.

- This commit adds new conversion test to verify if the external_url works fine. - This commit adds test cases in TestRenderManifest to verify if the prometheus external_url and prometheus ingress host matches. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd · 2020-10-16T07:39:34Z

Thanks @invidian @johananl for bearing with me and for the reviews. :-)

invidian previously approved these changes Sep 15, 2020

View reviewed changes

surajssd dismissed invidian’s stale review via c42040f September 22, 2020 09:47

surajssd force-pushed the surajssd/add-external-url branch 2 times, most recently from c42040f to d292347 Compare September 22, 2020 09:48

surajssd marked this pull request as ready for review September 22, 2020 09:49

invidian reviewed Sep 22, 2020

View reviewed changes

surajssd mentioned this pull request Sep 22, 2020

Add back the external_url in the prometheus block #826

Closed

invidian suggested changes Sep 22, 2020

View reviewed changes

surajssd force-pushed the surajssd/add-external-url branch from 0107dcb to 8efc6f5 Compare September 22, 2020 13:45

surajssd requested a review from ipochi September 23, 2020 12:40

invidian mentioned this pull request Sep 24, 2020

Create code style guide for Go, Terraform and Helm #883

Open

surajssd removed the request for review from ipochi September 24, 2020 11:42

invidian mentioned this pull request Sep 28, 2020

Update grafana and other prometheus operator sub components #963

Merged

3 tasks

surajssd force-pushed the surajssd/add-external-url branch 3 times, most recently from 6b3f2dd to c414107 Compare October 7, 2020 07:01

surajssd requested review from invidian and iaguis October 7, 2020 07:02

invidian suggested changes Oct 7, 2020

View reviewed changes

pkg/components/util/test/doc.go Outdated Show resolved Hide resolved

pkg/components/util/test/jsonpath.go Outdated Show resolved Hide resolved

surajssd force-pushed the surajssd/add-external-url branch from c414107 to 556ee55 Compare October 8, 2020 06:54

invidian suggested changes Oct 8, 2020

View reviewed changes

pkg/components/internal/jsonpath.go Outdated Show resolved Hide resolved

invidian previously requested changes Oct 8, 2020

View reviewed changes

surajssd force-pushed the surajssd/add-external-url branch from 556ee55 to a1cf893 Compare October 8, 2020 10:38

surajssd requested a review from johananl October 12, 2020 10:38

johananl reviewed Oct 12, 2020

View reviewed changes

surajssd force-pushed the surajssd/add-external-url branch from a1cf893 to be79773 Compare October 13, 2020 07:26

surajssd requested a review from johananl October 13, 2020 07:26

surajssd mentioned this pull request Oct 14, 2020

Add conversion tests for Velero #1075

Open

surajssd force-pushed the surajssd/add-external-url branch from be79773 to dbf93ea Compare October 14, 2020 08:27

prometheus-operator: Add external_url

2539257

This commit adds the feature removed sometime back called `external_url` this allows the user to choose where(URL) the prometheus is exposed. Signed-off-by: Suraj Deshmukh <suraj@kinvolk.io>

surajssd force-pushed the surajssd/add-external-url branch 2 times, most recently from 1ede981 to 79255db Compare October 15, 2020 12:00

invidian requested review from invidian and removed request for invidian October 15, 2020 14:46

johananl reviewed Oct 15, 2020

View reviewed changes

pkg/components/internal/testutil/common.go Outdated Show resolved Hide resolved

johananl previously approved these changes Oct 15, 2020

View reviewed changes

invidian previously approved these changes Oct 15, 2020

View reviewed changes

surajssd dismissed stale reviews from invidian and johananl via de70624 October 16, 2020 06:30

surajssd force-pushed the surajssd/add-external-url branch from 79255db to de70624 Compare October 16, 2020 06:30

surajssd requested a review from invidian October 16, 2020 07:29

invidian approved these changes Oct 16, 2020

View reviewed changes

surajssd merged commit ac60299 into master Oct 16, 2020

surajssd deleted the surajssd/add-external-url branch October 16, 2020 07:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prometheus-operator: Add external_url #964

prometheus-operator: Add external_url #964

surajssd commented Sep 14, 2020 •

edited

Loading

invidian left a comment

surajssd commented Sep 22, 2020 •

edited

Loading

invidian left a comment

surajssd commented Oct 5, 2020

invidian left a comment

invidian Oct 8, 2020

surajssd Oct 8, 2020

invidian Oct 8, 2020

surajssd Oct 8, 2020

invidian Oct 8, 2020

surajssd Oct 8, 2020

invidian Oct 8, 2020

surajssd Oct 8, 2020

johananl left a comment •

edited

Loading

johananl Oct 12, 2020

invidian Oct 12, 2020

surajssd Oct 13, 2020

johananl Oct 15, 2020

johananl Oct 12, 2020

surajssd Oct 13, 2020

johananl Oct 15, 2020

johananl commented Oct 12, 2020 •

edited

Loading

johananl left a comment •

edited

Loading

surajssd commented Oct 16, 2020

prometheus-operator: Add external_url #964

prometheus-operator: Add external_url #964

Conversation

surajssd commented Sep 14, 2020 • edited Loading

invidian left a comment

Choose a reason for hiding this comment

surajssd commented Sep 22, 2020 • edited Loading

invidian left a comment

Choose a reason for hiding this comment

surajssd commented Oct 5, 2020

invidian 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

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

johananl left a comment • edited Loading

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

johananl commented Oct 12, 2020 • edited Loading

johananl left a comment • edited Loading

Choose a reason for hiding this comment

surajssd commented Oct 16, 2020

surajssd commented Sep 14, 2020 •

edited

Loading

surajssd commented Sep 22, 2020 •

edited

Loading

johananl left a comment •

edited

Loading

johananl commented Oct 12, 2020 •

edited

Loading

johananl left a comment •

edited

Loading