Change CRD to use lower camel case

[objectiser]

Changed all-in-one key (and strategy value) to use allInOne.

Changed cassandra-create-schema to be cassandraCreateSchema.

Signed-off-by: Gary Brown [email protected]

[jpkrohling]

This change is 

[jpkrohling]

I'm surprised that there are only a few changes required for this! It's better than I thought :-) I have only one small minor request, LGTM otherwise.

Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @objectiser)

---

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

	logrus.Debugf("Jaeger strategy: %s", jaeger.Spec.Strategy)
	if jaeger.Spec.Strategy == "allInOne" {

Perhaps we could have another conditional here, setting the strategy to allInOne if it's all-in-one? This way, we don't break current CRs using strategy=all-on-one. We could possibly log a WARN, stating that all-in-one is deprecated in favor of allInOne.

[codecov]

Codecov Report

Merging #87 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   99.35%   99.35%   +<.01%     
==========================================
  Files          18       18              
  Lines         773      776       +3     
==========================================
+ Hits          768      771       +3     
  Misses          5        5

Impacted Files	Coverage Δ
pkg/controller/controller.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 8891ff5...a58d9e2. Read the comment docs.

[jpkrohling]

Looks like the coverage went down. Would you mind checking what's missing?

[jpkrohling]

Just a small typo on the new test, otherwise. One thing we could do to not break compatibility with older CRs is to duplicate the spec entries for CassandraCreateSchema and AllInOne.

I'm not sure it's worth at this point, but it's something else to consider in the future (apart from creating a new spec version, like v1alpha2)

Reviewed 1 of 1 files at r2, 2 of 2 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @objectiser)

---

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

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

Perhaps we could have another conditional here, setting the strategy to allInOne if it's all-in-one? This way, we don't break current CRs using strategy=all-on-one. We could possibly log a WARN, stating that all-in-one is deprecated in favor of allInOne.

Done, thanks!

---

pkg/controller/controller_test.go, line 100 at r3 (raw file):

}

func TestDreprecatedAllInOneStrategy(t *testing.T) {

s/Dreprecated/Deprecated

[objectiser]

@jpkrohling Agree. We need to decide when the operator is stable enough to start maintaining backward compatibility and using spec version to make significant changes. We should also do a release soon and document the breaking changes.

[jpkrohling]

Sorry for the last minute change, but I think I found a possible bug that would be introduced by this change involving ToLower.

Reviewed 4 of 4 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @objectiser)

---

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

// NewController build a new controller object for the given spec
func NewController(ctx context.Context, jaeger *v1alpha1.Jaeger) Controller {
	if jaeger.Spec.Strategy == "all-in-one" {

nit: strings.ToLower(jaeger.Spec.Strategy)

---

pkg/controller/controller.go, line 70 at r4 (raw file):

	// check for incompatible options
	// if the storage is `memory`, then the only possible strategy is `all-in-one`
	if strings.ToLower(jaeger.Spec.Storage.Type) == "memory" && strings.ToLower(jaeger.Spec.Strategy) != "allInOne" {

I missed this before: the second part will never match, as the right-hand side comparison is lowering the case for the strategy but uses upper case in the constant. In other words, ToLower("allInOne") will never be equal to "allInOne".

I guess we need a new test :)

---

pkg/controller/controller_test.go, line 100 at r3 (raw file):

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

s/Dreprecated/Deprecated

Thanks!

[jpkrohling]

Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @objectiser)

[jpkrohling]

Reviewable status: complete! all files reviewed, all discussions resolved