support volumes and volumeMounts

[clyang82]

This PR is to fix the issue - #76
This goal is to support add volumes and volumeMounts in kind Jaeger

Signed-off-by: Chun Lin Yang [email protected]

[jpkrohling]

This change is 

[codecov]

Codecov Report

Merging #82 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
+ Coverage    99.4%   99.41%   +0.01%     
==========================================
  Files          20       21       +1     
  Lines         837      861      +24     
==========================================
+ Hits          832      856      +24     
  Misses          5        5

Impacted Files	Coverage Δ
pkg/deployment/query.go	100% <100%> (ø)	⬆️
pkg/deployment/all-in-one.go	100% <100%> (ø)	⬆️
pkg/deployment/collector.go	100% <100%> (ø)	⬆️
pkg/util/util.go	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 04af229...0de1241. Read the comment docs.

[jpkrohling]

Reviewed 5 of 5 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @clyang82)

a discussion (no related file):
Thanks, this looks good! I'd like to see a couple of changes, though:

1. a new example file under deploy/examples
2. documentation on the README file
3. a couple of unit tests
4. not sure the new options are relevant for the agent, better not have it there if there's no use

---

---

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

	Image        string           `json:"image"`
	Options      Options          `json:"options"`
	Volumes      []v1.Volume      `json:"volumes"`

Right now, we don't need this in the agent, as there are no options for it that would load files, except --config-file (which is not recommended with the operator).

---

pkg/deployment/agent.go, line 83 at r1 (raw file):

						Name:         "jaeger-agent-daemonset",
						Args:         args,
						VolumeMounts: append(a.jaeger.Spec.VolumeMounts, a.jaeger.Spec.Agent.VolumeMounts...),

Could you add a couple of tests asserting this precedence? If two volumes with the same name exist, what would happen? This applies to the similar changes in the other constructs as well.

---

pkg/deployment/agent.go, line 119 at r1 (raw file):

						},
					}},
					Volumes: append(a.jaeger.Spec.Volumes, a.jaeger.Spec.Agent.Volumes...),

Same here: please add a couple of tests asserting the precedence and showing what happens when two volumes with the same required parameters exist (name?). This applies to the similar changes in the other constructs as well.

[clyang82]

@jpkrohling Thanks for your comments. most of them were addressed today except for document update. I will update tomorrow.

[clyang82]

Reviewable status: 1 of 10 files reviewed, 4 unresolved discussions (waiting on @jpkrohling)

a discussion (no related file):

Right now, we don't need this in the agent, as there are no options for it that would load files, except --config-file (which is not recommended with the operator).

Thanks for your comment. I will remove the changes in the agent.

---

---

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

Could you add a couple of tests asserting this precedence? If two volumes with the same name exist, what would happen? This applies to the similar changes in the other constructs as well.

Added some test cases. please review.

Handle the case for the same name. if two volumes with the same name exist, remove one of them. more concretely, if there are the same name volume in Jaeger and Query, use the one in Query and remove the one in Jaeger (global one)

---

pkg/deployment/agent.go, line 83 at r1 (raw file):

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

Could you add a couple of tests asserting this precedence? If two volumes with the same name exist, what would happen? This applies to the similar changes in the other constructs as well.

Done.

---

pkg/deployment/agent.go, line 119 at r1 (raw file):

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

Same here: please add a couple of tests asserting the precedence and showing what happens when two volumes with the same required parameters exist (name?). This applies to the similar changes in the other constructs as well.

Done.

[clyang82]

Reviewable status: 1 of 10 files reviewed, 4 unresolved discussions (waiting on @jpkrohling)

a discussion (no related file):

Previously, clyang82 (Chunlin Yang) wrote…

Right now, we don't need this in the agent, as there are no options for it that would load files, except --config-file (which is not recommended with the operator).

Thanks for your comment. I will remove the changes in the agent.

Done.

---

---

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

Previously, clyang82 (Chunlin Yang) wrote…

Could you add a couple of tests asserting this precedence? If two volumes with the same name exist, what would happen? This applies to the similar changes in the other constructs as well.

Added some test cases. please review.

Handle the case for the same name. if two volumes with the same name exist, remove one of them. more concretely, if there are the same name volume in Jaeger and Query, use the one in Query and remove the one in Jaeger (global one)

Done.

[jpkrohling]

Looks really good, I think we are very close to getting this merged. I have only a couple of remaining comments, including a very small one.

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

---

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

	// AllInOne is first while global is second
	for index, volume := range podSpec.Volumes {

It might be cleaner to do something like:

assert.Equal(t, "allInOneVolume", podSpec.Volumes[0].Name)
assert.Equal(t, "globalVolume", podSpec.Volumes[1].Name)

---

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

	}

	for index, volumeMount := range podSpec.Containers[0].VolumeMounts {

Same as above.

---

pkg/deployment/collector_test.go, line 116 at r2 (raw file):

	// collector is first while global is second
	for index, volume := range podSpec.Volumes {
		if index == 0 {

Same as a previous comment.

---

pkg/deployment/collector_test.go, line 123 at r2 (raw file):

	}

	for index, volumeMount := range podSpec.Containers[0].VolumeMounts {

Ditto

---

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

	for index, volume := range podSpec.Volumes {
		if index == 0 {
			assert.Equal(t, "queryVolume", volume.Name)

And here :)

---

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

	}

	for index, volumeMount := range podSpec.Containers[0].VolumeMounts {

Here too

---

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

}
func TestQueryVolumeMountsWithSameName(t *testing.T) {

nit: could you add an empty line here, between the closing bracket and the new func? There are a couple of other places that could also use a new line after the bracket :)

---

pkg/util/util.go, line 7 at r2 (raw file):

)

// RemoveDuplicatedVolumes defines to remove the duplicated item in slice if the Name of Volume is the same

Please add which item is expected to be kept. Like: "the first item wins" or "the last item wins", as I can see a valid argument for any one of those cases.

---

pkg/util/util.go, line 12 at r2 (raw file):

	var result []v1.Volume

	for i := 0; i < len(volumes); i++ {

You could keep a map of booleans and check it before appending. Like:

existing := map[string]bool{}

for _, v ... {
  if existing[v.Name] {
    continue
  }
  results = append(...)
  existing[v.Name] = true
}

This way, you don't need the inner loop.

---

pkg/util/util.go, line 35 at r2 (raw file):

	var result []v1.VolumeMount

	for i := 0; i < len(volumeMounts); i++ {

Same as above.

---

pkg/util/util_test.go, line 44 at r2 (raw file):

	}

	assert.Len(t, RemoveDuplicatedVolumeMounts(volumeMounts), 2)

Can you also add an assertion to make it explicit which item is expected to be kept/removed? Same for the previous test.

[clyang82]

@jpkrohling Thanks for your time to review my PR. All your comments were addressed. I would like to open another PR for document update so that the PR is ready for merge if no further comments.

[jpkrohling]

There's only one thing that needs to be changed, related to the rebase overwriting a recent change. All other requests are minor and won't block merging this.

Reviewed 9 of 11 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @clyang82)

---

pkg/apis/io/v1alpha1/types.go, line 38 at r4 (raw file):

type JaegerSpec struct {
	Strategy     string              `json:"strategy"`
	AllInOne     JaegerAllInOneSpec  `json:"all-in-one"`

I think the last rebase is overwriting a change that was recently merged:
#87

---

pkg/deployment/query_test.go, line 7 at r4 (raw file):

	"testing"

	"github.com/jaegertracing/jaeger-operator/pkg/apis/io/v1alpha1"

nit: per convention, the imports related to the same project are part of the third group

---

pkg/deployment/query_test.go, line 10 at r4 (raw file):

	"github.com/spf13/viper"
	"github.com/stretchr/testify/assert"
	"k8s.io/api/core/v1"

nit: per convention, imports to dependencies are part of the second group

---

pkg/util/util.go, line 7 at r2 (raw file):

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

Please add which item is expected to be kept. Like: "the first item wins" or "the last item wins", as I can see a valid argument for any one of those cases.

nit: RemoveDuplicatedVolumes returns a unique list of Volumes based on Volume names. Only the first item is kept.

---

pkg/util/util.go, line 23 at r4 (raw file):

}

// RemoveDuplicatedVolumeMounts defines to remove the last duplicated item in slice if the Name of Volume is the same

nit: RemoveDuplicatedVolumeMounts returns a unique list based on the item names. Only the first item is kept.

I'm merging this PR as is, as I could make use of this in a PR I'm working on (related to #89)

[jpkrohling]

Thanks for your contribution, @clyang82 !

[clyang82]

the review comments were fixed in #98. Thanks for your help. @jpkrohling