adding configurable version in JaegerSpec #174

rajdhandus · 2019-01-23T01:40:36Z

Please review and provide feedback..

jpkrohling · 2019-01-23T01:40:40Z

This change is

Signed-off-by: Rajkumar Purushothaman <[email protected]>

jpkrohling · 2019-01-23T09:26:03Z

Looks like the build is failing with:

Go fmt, license check, or import ordering failures, run 'make format'

jpkrohling

Reviewed 34 of 34 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @rajdhandus)

a discussion (no related file):
The core of this PR is looking good! There are some things to improve (see the individual comments), but nothing really serious. Thanks for your contribution!

README.adoc, line 125 at r1 (raw file):

  name: simplest
spec:
  version: "123"

Use a real value, like "1.8". It makes it easier for people to relate this value to something they know.

README.adoc, line 138 at r1 (raw file):

  name: my-jaeger
spec:
  version: "123"

Same as above, use "1.8" instead of a version that doesn't exist anywhere.

deploy/examples/agent-as-daemonset.yaml, line 6 at r1 (raw file):

  name: agent-as-daemonset
spec:
  version: "123"

I'd keep the example as it was, given that version isn't required.

deploy/examples/all-in-one-with-options.yaml, line 6 at r1 (raw file):

  name: "my-jaeger"
spec:
  version: "123"

Same as the previous comment (and valid for all other examples): let's not add the version here.

deploy/examples/simplest-with-version.yaml, line 4 at r1 (raw file):

kind: Jaeger
metadata:
  name: simplest

Name the file with-version.yaml and change the name: from simplest to with-version.

deploy/examples/simplest-with-version.yaml, line 6 at r1 (raw file):

  name: simplest
spec:
  version: "123"

s/123/1.8

pkg/apis/io/v1alpha1/builder.go, line 17 at r1 (raw file):

// NewJaegerVersion returns a new Jaeger instance with the given name and a version
func NewJaegerVersion(name string, version string) *Jaeger {

Not sure I agree with the inclusion here, as the constructor accepts only required values, which isn't the case for the version. Our current pattern is to create a Jaeger object based on the name and then customize before using it. Like this, from pkg/strategy/all-in-one_test.go:

	j := v1alpha1.NewJaeger(name)
	j.Spec.Agent.Strategy = "DaemonSet"

	c := newAllInOneStrategy(context.TODO(), j)

pkg/strategy/controller.go, line 53 at r1 (raw file):

	// normalize the version
	if jaeger.Spec.Version == "" {
		jaegerVersion := viper.GetString("jaeger-version")

You can avoid having this var by making it look like the following:

jaeger.Spec.Version = viper.GetString("jaeger-version")
logrus.Infof("Version wasn't provided for the Jaeger instance '%v'. Falling back to '%v'", jaeger.Name, jaeger.Spec.Version)

test/e2e/all_in_one_test.go, line 16 at r1 (raw file):

	"github.com/sirupsen/logrus"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"k8s.io/apimachinery/pkg/util/wait"

I suspect you are not using make format. If you are indeed using it, let me know, as this change shouldn't have been part of the PR. This also applies to other changes.

Signed-off-by: Rajkumar Purushothaman <[email protected]>

jpkrohling

Reviewed 10 of 13 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @rajdhandus)

a discussion (no related file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

The core of this PR is looking good! There are some things to improve (see the individual comments), but nothing really serious. Thanks for your contribution!

Some formatting changes need to be reverted, as well as a couple of changes to the tests ("default image" tests have to be reverted, a new test for the new functionality has to be added).

It's getting close, though :-)

deploy/examples/with-cassandra.yaml, line 6 at r2 (raw file):

  name: with-cassandra
spec:
  version: "123"

Please, remove this and similar changes from the examples.

deploy/examples/with-sampling.yaml, line 6 at r2 (raw file):

  name: with-sampling
spec:
  version: "123"

Here as well.

deploy/examples/openshift/disable-oauth-proxy.yaml, line 6 at r2 (raw file):

  name: disable-oauth-proxy
spec:
  version: "123"

Here as well.

pkg/apis/io/v1alpha1/builder.go, line 5 at r2 (raw file):

import (
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

Did make format made this change?

pkg/deployment/query_test.go, line 43 at r2 (raw file):

	jaeger := v1alpha1.NewJaeger("TestQueryImage")
	jaeger.Spec.Version = "123"

I just realized that this test (and others in this PR) are about the default image. So, it should be as it was before.

pkg/strategy/controller.go, line 52 at r2 (raw file):

	// normalize the version
	if jaeger.Spec.Version == "" {

I'm missing some tests for this new behavior. Could you please create two tests:

With viper.Set("jaeger-version", "1.1"), the Spec.Version should be 1.1
Without anything, Spec.Version shouldn't be empty (do not assume a specific value, as the default comes from the file jaeger.version.

…ator into feature-166

Signed-off-by: Rajkumar Purushothaman <[email protected]>

rajdhandus

Reviewable status: 15 of 16 files reviewed, 7 unresolved discussions (waiting on @jpkrohling and @rajdhandus)

a discussion (no related file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

Some formatting changes need to be reverted, as well as a couple of changes to the tests ("default image" tests have to be reverted, a new test for the new functionality has to be added).

It's getting close, though :-)

thanks for your patience.. i don't think i added any "default image" tests... can you please clarify?

test/e2e/all_in_one_test.go, line 16 at r1 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

I suspect you are not using make format. If you are indeed using it, let me know, as this change shouldn't have been part of the PR. This also applies to other changes.

I ran make format.. it doesn't seem to have any effect on the files... i am using a mac..

rpurusho:jaeger-operator rpurusho$ make format
Formatting code...
rpurusho:jaeger-operator rpurusho$

rajdhandus

Reviewable status: 15 of 16 files reviewed, 7 unresolved discussions (waiting on @jpkrohling and @rajdhandus)

a discussion (no related file):

Previously, rajdhandus (Rajkumar Purushothaman) wrote…

thanks for your patience.. i don't think i added any "default image" tests... can you please clarify?

also - about the possibility for the new test, what would be the right place for the new test? this change is about adding version to the common spec.. i am wondering if i should create a jaeger object, set the version and assert the same? checked how the other properties in the common spec are tested.. couldn't find any..

jpkrohling

Reviewed 3 of 15 files at r3.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @rajdhandus)

a discussion (no related file):

Previously, rajdhandus (Rajkumar Purushothaman) wrote…

also - about the possibility for the new test, what would be the right place for the new test? this change is about adding version to the common spec.. i am wondering if i should create a jaeger object, set the version and assert the same? checked how the other properties in the common spec are tested.. couldn't find any..

We are very close now! I added comments to all the places that need changing, hopefully being clearer on what needs to be done, including where the test should be.

In summary, we don't test the structs (the new Version field, for instance), but we do test any new behavior that uses this field. We are changing the logic that builds what's the image to use, so, we want tests assuring the new code is doing what we want it to do, like:

When the CLI option --jaeger-version is passed and the CR has a version specified, the CR version wins
When the CLI option is specified but no version the CR is present, the CLI option is used
When none is present (CLI option/CR), a default value is used (ie, not empty)

pkg/deployment/agent_test.go, line 40 at r3 (raw file):

	jaeger := v1alpha1.NewJaeger("TestNewAgent")
	jaeger.Spec.Version = "123"

Please, revert this change. This test is about testing the default image, which is derived from the command line options being passed.

pkg/deployment/all-in-one_test.go, line 24 at r3 (raw file):

	jaeger := v1alpha1.NewJaeger("TestAllInOneDefaultImage")
	jaeger.Spec.Version = "123"

Please, revert this change. This test is about testing the default image, which is derived from the command line options being passed.

pkg/deployment/collector_test.go, line 55 at r3 (raw file):

	jaeger := v1alpha1.NewJaeger("TestCollectorImage")
	jaeger.Spec.Version = "123"

Please, revert this change. This test is about testing the default image, which is derived from the command line options being passed.

pkg/deployment/query_test.go, line 43 at r2 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

I just realized that this test (and others in this PR) are about the default image. So, it should be as it was before.

Same as the other similar changes: please, revert this change. This test is about testing the default image, which is derived from the command line options being passed.

pkg/strategy/controller.go, line 52 at r2 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

I'm missing some tests for this new behavior. Could you please create two tests:

With viper.Set("jaeger-version", "1.1"), the Spec.Version should be 1.1

Without anything, Spec.Version shouldn't be empty (do not assume a specific value, as the default comes from the file jaeger.version.

The test should be in the pkg/strategy/controller_test.go, as it will test the behavior added to pkg/strategy/controller.go

test/e2e/all_in_one_test.go, line 16 at r1 (raw file):

Previously, rajdhandus (Rajkumar Purushothaman) wrote…

I ran make format.. it doesn't seem to have any effect on the files... i am using a mac..

rpurusho:jaeger-operator rpurusho$ make format
Formatting code...
rpurusho:jaeger-operator rpurusho$

Ok, I might have looked at a previous revision. It looks good as it is now.

rajdhandus

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jpkrohling and @rajdhandus)

deploy/examples/with-cassandra.yaml, line 6 at r2 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

Please, remove this and similar changes from the examples.

Done.

deploy/examples/with-sampling.yaml, line 6 at r2 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

Here as well.

Done.

deploy/examples/openshift/disable-oauth-proxy.yaml, line 6 at r2 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

Here as well.

Done.

pkg/deployment/query_test.go, line 43 at r2 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

Same as the other similar changes: please, revert this change. This test is about testing the default image, which is derived from the command line options being passed.

the reason why i did this was so that the new Jaeger object that is created would have the version set.. NewJaeger("") doesn't read from the command line and set the version.. it used to.. i had changed that because of the original comment you had in the issue..

"Change all usages of viper.GetString("jaeger-version") to get the version from the newly created property jaeger.Spec.Version "

pkg/strategy/controller.go, line 52 at r2 (raw file):

as the default comes from the file jaeger.version

Can you please clarify this statement? where exactly is jaeger.version?

jpkrohling

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @rajdhandus)

deploy/examples/with-cassandra.yaml, line 6 at r2 (raw file):

Previously, rajdhandus (Rajkumar Purushothaman) wrote…

Done.

It's still there :-)

deploy/examples/with-sampling.yaml, line 6 at r2 (raw file):

Previously, rajdhandus (Rajkumar Purushothaman) wrote…

Done.

It's still there

deploy/examples/openshift/disable-oauth-proxy.yaml, line 6 at r2 (raw file):

Previously, rajdhandus (Rajkumar Purushothaman) wrote…

Done.

It's still there

pkg/deployment/query_test.go, line 43 at r2 (raw file):

Previously, rajdhandus (Rajkumar Purushothaman) wrote…

the reason why i did this was so that the new Jaeger object that is created would have the version set.. NewJaeger("") doesn't read from the command line and set the version.. it used to.. i had changed that because of the original comment you had in the issue..

"Change all usages of viper.GetString("jaeger-version") to get the version from the newly created property jaeger.Spec.Version "

Right, the change all usages... comment was in reference to the actual code, not tests. The tests are representing how the operator is used, and here, it means that the version is configured via command line (or configuration file, or ..., this is abstracted away to us via viper).

pkg/strategy/controller.go, line 52 at r2 (raw file):

Previously, rajdhandus (Rajkumar Purushothaman) wrote…

as the default comes from the file jaeger.version

Can you please clarify this statement? where exactly is jaeger.version?

When nothing is specified, the version to use is whatever is specified in the file jaeger.version, present in the root of the project. During the build, the contents of this are then set to pkg/version/main.go.

rajdhandus

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @jpkrohling and @rajdhandus)

pkg/deployment/query_test.go, line 43 at r2 (raw file):

the reason why i did this was so that the new Jaeger object that is created would have the version set.. NewJaeger("") doesn't read from the command line and set the version.. it used to.. i had changed that because of the original comment you had in the issue..

Okay got it.. thanks..

rajdhandus

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @jpkrohling and @rajdhandus)

pkg/deployment/agent.go, line 26 at r3 (raw file):

func NewAgent(jaeger *v1alpha1.Jaeger) *Agent {
	if jaeger.Spec.Agent.Image == "" {
		jaeger.Spec.Agent.Image = fmt.Sprintf("%s:%s", viper.GetString("jaeger-agent-image"), viper.GetString("jaeger-version"))

Should this be reverted ? I feel that this logic needs to be fixed.. the Jaeger Object that is passed in just has name... It used to get the version from viper.. after my change - where I tried to replace all viper.GetString("jaeger-version")... it seems to create agent object without the version part.. so existing test cases are failing.. Should I maybe check if the version is empty and get it from viper if it is?

jpkrohling

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @jpkrohling and @rajdhandus)

pkg/deployment/agent.go, line 26 at r3 (raw file):

Previously, rajdhandus (Rajkumar Purushothaman) wrote…

Should this be reverted ? I feel that this logic needs to be fixed.. the Jaeger Object that is passed in just has name... It used to get the version from viper.. after my change - where I tried to replace all viper.GetString("jaeger-version")... it seems to create agent object without the version part.. so existing test cases are failing.. Should I maybe check if the version is empty and get it from viper if it is?

No, the jaeger instance received here has to be passed through the normalize() function before, which will place the correct value there. jaeger.Spec.Version should never be empty at this point.

The tests that existed before this PR should still work as they were, and a new test should be added to pkg/strategy/controller_test.go testing the scenarios I described before.

rajdhandus

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @jpkrohling and @rajdhandus)

deploy/examples/with-cassandra.yaml, line 6 at r2 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

It's still there :-)

sorry.. i have not pushed it up..

deploy/examples/with-sampling.yaml, line 6 at r2 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

It's still there

same.. have not pushed it up..spoke too soon..

deploy/examples/openshift/disable-oauth-proxy.yaml, line 6 at r2 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

It's still there

same..

pkg/deployment/agent.go, line 26 at r3 (raw file):

Should this be reverted ? I feel that this logic needs to be fixed.. the Jaeger Object that is passed in just has name... It used to get the version from viper.. after my change - where I tried to replace all viper.GetString("jaeger-version")... it seems to create agent object without the version part.. so existing test cases are failing.. Should I maybe check if the version is empty and get it from viper if it is?

Quoted 16 lines of code…

              7 hours ago
            
          
          
            New comments
            mark read
          
          
          jpkrohlingJuraci Paixão Kröhling

So the normalize() function should be called before calling NewAgent() ? ___ *[pkg/deployment/agent_test.go, line 40 at r3](https://reviewable.io/reviews/jaegertracing/jaeger-operator/174#-LWyx_RVBgbDdd46-YNe:-LX54bHP-VBJU3Ai-oEJ:bb7oryy) ([raw file](https://github.com/jaegertracing/jaeger-operator/blob/121a7fac41c82e6394045855b90d08c96b374e43/pkg/deployment/agent_test.go#L40)):*

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

Please, revert this change. This test is about testing the default image, which is derived from the command line options being passed.

sure

pkg/deployment/all-in-one_test.go, line 24 at r3 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

Please, revert this change. This test is about testing the default image, which is derived from the command line options being passed.

sure

pkg/deployment/collector_test.go, line 55 at r3 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

Please, revert this change. This test is about testing the default image, which is derived from the command line options being passed.

sure

jpkrohling

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @jpkrohling and @rajdhandus)

pkg/deployment/agent.go, line 26 at r3 (raw file):

Previously, rajdhandus (Rajkumar Purushothaman) wrote…

Should this be reverted ? I feel that this logic needs to be fixed.. the Jaeger Object that is passed in just has name... It used to get the version from viper.. after my change - where I tried to replace all viper.GetString("jaeger-version")... it seems to create agent object without the version part.. so existing test cases are failing.. Should I maybe check if the version is empty and get it from viper if it is?
              7 hours ago
            
          
          
            New comments
            mark read
          
          
          jpkrohlingJuraci Paixão Kröhling
So the normalize() function should be called before calling NewAgent() ?

The normalize() function is already called before the NewAgent() is called. It happens either in the code from the controller package, which is called whenever a new Jaeger instance is created, or in the tests before calling NewAgent(), like here:

https://github.com/jaegertracing/jaeger-operator/blob/master/pkg/strategy/controller_test.go#L53-L58

jpkrohling

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @jpkrohling and @rajdhandus)

pkg/deployment/agent.go, line 26 at r3 (raw file):

Previously, jpkrohling (Juraci Paixão Kröhling) wrote…

The normalize() function is already called before the NewAgent() is called. It happens either in the code from the controller package, which is called whenever a new Jaeger instance is created, or in the tests before calling NewAgent(), like here:

https://github.com/jaegertracing/jaeger-operator/blob/master/pkg/strategy/controller_test.go#L53-L58

About the agent_test.go, it should be like it was before, so, no normalize() call is needed there. Testing of the new code should happen in the controller_test.go.

jpkrohling · 2019-02-22T12:01:17Z

@rajdhandus , would you need some help in moving with this PR?

jpkrohling · 2019-02-28T08:38:36Z

@rajdhandus, looks like there's only a couple of minor things to get this merged. Would you like to continue working on this?

adding configurable version in JaegerSpec

2863155

Signed-off-by: Rajkumar Purushothaman <[email protected]>

rajdhandus force-pushed the feature-166 branch from cd02e46 to 2863155 Compare January 23, 2019 01:42

running fmt on all go files

f9a2ae5

Signed-off-by: Rajkumar Purushothaman <[email protected]>

rajdhandus force-pushed the feature-166 branch from b2c366b to f9a2ae5 Compare January 23, 2019 02:12

jpkrohling reviewed Jan 23, 2019

View reviewed changes

incorporating changes from review..

2db2b24

Signed-off-by: Rajkumar Purushothaman <[email protected]>

rajdhandus force-pushed the feature-166 branch from 874f01a to 2db2b24 Compare January 23, 2019 10:57

jpkrohling reviewed Jan 23, 2019

View reviewed changes

Rajkumar Purushothaman added 2 commits January 23, 2019 20:16

Merge branch 'master' of https://github.com/jaegertracing/jaeger-oper…

432aa49

…ator into feature-166

revert the formatting changes

121a7fa

Signed-off-by: Rajkumar Purushothaman <[email protected]>

rajdhandus commented Jan 24, 2019

View reviewed changes

jpkrohling reviewed Jan 24, 2019

View reviewed changes

rajdhandus commented Jan 24, 2019

View reviewed changes

jpkrohling reviewed Jan 24, 2019

View reviewed changes

rajdhandus commented Jan 24, 2019

View reviewed changes

jpkrohling reviewed Jan 25, 2019

View reviewed changes

rajdhandus commented Jan 25, 2019

View reviewed changes

jpkrohling reviewed Jan 28, 2019

View reviewed changes

jpkrohling closed this Mar 21, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding configurable version in JaegerSpec #174

adding configurable version in JaegerSpec #174

rajdhandus commented Jan 23, 2019

jpkrohling commented Jan 23, 2019

jpkrohling commented Jan 23, 2019

jpkrohling left a comment

jpkrohling left a comment

rajdhandus left a comment

rajdhandus left a comment

jpkrohling left a comment

rajdhandus left a comment

jpkrohling left a comment

rajdhandus left a comment

rajdhandus left a comment

jpkrohling left a comment

rajdhandus left a comment

jpkrohling left a comment

jpkrohling left a comment

jpkrohling commented Feb 22, 2019

jpkrohling commented Feb 28, 2019

adding configurable version in JaegerSpec #174

adding configurable version in JaegerSpec #174

Conversation

rajdhandus commented Jan 23, 2019

jpkrohling commented Jan 23, 2019

jpkrohling commented Jan 23, 2019

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

rajdhandus left a comment

Choose a reason for hiding this comment

rajdhandus left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

rajdhandus left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

rajdhandus left a comment

Choose a reason for hiding this comment

rajdhandus left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

rajdhandus left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling commented Feb 22, 2019

jpkrohling commented Feb 28, 2019