Moved from v1alpha1 to v1 #265

jpkrohling · 2019-03-05T15:30:36Z

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling · 2019-03-05T15:30:47Z

This change is

jpkrohling · 2019-03-05T15:31:22Z

This PR is still missing the warning in case people try to use the old v1alpha1. This will be done in a subsequent commit to this PR, but it would be nice to get a first feedback on the changes so far.

codecov · 2019-03-05T15:34:08Z

Codecov Report

Merging #265 into master will decrease coverage by 0.14%.
The diff coverage is 93.54%.

@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
- Coverage   89.05%   88.91%   -0.15%     
==========================================
  Files          63       68       +5     
  Lines        2823     2886      +63     
==========================================
+ Hits         2514     2566      +52     
- Misses        201      211      +10     
- Partials      108      109       +1

Impacted Files	Coverage Δ
pkg/apis/jaegertracing/v1/logger.go	`0% <0%> (ø)`
pkg/apis/jaegertracing/v1/builder.go	`0% <0%> (ø)`
pkg/controller/jaeger/role.go	`63.63% <100%> (ø)`	⬆️
pkg/strategy/streaming.go	`100% <100%> (ø)`	⬆️
pkg/route/query.go	`100% <100%> (ø)`	⬆️
pkg/strategy/production.go	`75.4% <100%> (ø)`	⬆️
pkg/deployment/collector.go	`100% <100%> (ø)`	⬆️
pkg/deployment/ingester.go	`100% <100%> (ø)`	⬆️
pkg/ingress/query.go	`100% <100%> (ø)`	⬆️
pkg/account/oauth-proxy.go	`100% <100%> (ø)`	⬆️
... and 43 more

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 7088dcf...9d48000. Read the comment docs.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling · 2019-03-05T16:00:46Z

When a CR with the old apiVersion is deployed, this is what's shown in the operator's logs:

WARN[0000] Attention: you are using a legacy CR for Jaeger. Use the apiVersion 'jaegertracing.io/v1' instead of 'io.jaegertracing/v1alpha1'  instance=simplest namespace=default

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

objectiser

Only some minor comments. Just want to clarify - so the current PR will print a warning and not deploy anything, but before we release v1 it will handle deploying based on the old alpha1 version aswell?

objectiser · 2019-03-05T18:56:53Z

CONTRIBUTING.adoc

@@ -90,7 +90,7 @@ Similar instructions also work for OpenShift, but the target `run-openshift` can

 ==== Model changes

-The Operator SDK generates the `pkg/apis/io/v1alpha1/zz_generated.deepcopy.go` file via the command `make generate`. This should be executed whenever there's a model change (`pkg/apis/io/v1alpha1/types.go`)
+The Operator SDK generates the `pkg/apis/jaegertracing/v1beta1/zz_generated.deepcopy.go` file via the command `make generate`. This should be executed whenever there's a model change (`pkg/apis/jaegertracing/v1beta1/jaeger_types.go`)


Shouldn't it be /v1 rather than /v1beta1?

objectiser · 2019-03-05T19:20:28Z

test/e2e/sidecar.go

@@ -161,6 +161,7 @@ func sidecarTest(t *testing.T, f *framework.Framework, ctx *framework.TestCtx) e
 		resp := &resp{}
 		err = json.Unmarshal(body, &resp)
 		if err != nil {
+			fmt.Println(body)


Did you mean to have this Println?

jpkrohling · 2019-03-06T06:09:10Z

Just want to clarify - so the current PR will print a warning and not deploy anything, but before we release v1 it will handle deploying based on the old alpha1 version aswell?

We could do some type transformation. My preference would be to not have it for v1alpha1 -> v1, only for v1 -> v1.next and on.

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

pavolloffay

Do we need to keep apis.go for v1aplha1 if we don't do model transhormation?

It seems that some files reference io.jaegertracing. Like the OLM catalog

.travis/release.sh:22:sed "s~containerImage: docker.io/jaegertracing/jaeger-operator.*~containerImage: docker.io/${BUILD_IMAGE}~gi" -i deploy/olm-catalog/jaeger-operator.csv.yaml
CONTRIBUTING.adoc:116:jaeger.io.jaegertracing/simplest created
deploy/crds/io_v1alpha1_jaeger_cr.yaml:2:apiVersion: io.jaegertracing/v1alpha1
deploy/crds/io_v1alpha1_jaeger_crd.yaml:4:  name: jaegers.io.jaegertracing
deploy/crds/io_v1alpha1_jaeger_crd.yaml:6:  group: io.jaegertracing
deploy/olm-catalog/_generated.concat_crd.yaml:4:  name: jaegers.io.jaegertracing
deploy/olm-catalog/_generated.concat_crd.yaml:6:  group: io.jaegertracing
deploy/olm-catalog/jaeger-operator.csv.yaml:57:    containerImage: docker.io/jaegertracing/jaeger-operator:1.10.0
deploy/olm-catalog/jaeger-operator.csv.yaml:70:      name: jaegers.io.jaegertracing
deploy/olm-catalog/jaeger-operator.csv.yaml:109:          - io.jaegertracing
deploy/olm-catalog/jaeger-operator.csv.yaml:199:          - io.jaegertracing
deploy/role.yaml:38:  - io.jaegertracing
pkg/apis/io/v1alpha1/doc.go:3:// +groupName=io.jaegertracing
pkg/apis/io/v1alpha1/register.go:5:// +groupName=io.jaegertracing
pkg/apis/io/v1alpha1/register.go:15:    SchemeGroupVersion = schema.GroupVersion{Group: "io.jaegertracing", Version: "v1alpha1"}
pkg/controller/legacy/legacy_controller.go:80:  }).Warn("Attention: you are using a legacy CR for Jaeger. Use the apiVersion 'jaegertracing.io/v1' instead of 'io.jaegertracing/v1alpha1'")
test/role.yaml:37:  - io.jaegertracing

CONTRIBUTING.adoc

deploy/crds/jaegertracing_v1_jaeger_cr.yaml

deploy/crds/jaegertracing_v1_jaeger_crd.yaml

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling · 2019-03-06T08:28:00Z

Do we need to keep apis.go for v1aplha1 if we don't do model transhormation?

Yes, otherwise users will get the following error when trying to create a Jaeger instance with the old apiGroup:

error: unable to recognize "/tmp/simplest.yaml": no matches for kind "Jaeger" in version "io.jaegertracing/v1"

It seems that some files reference io.jaegertracing. Like the OLM catalog

I fixed the OLM catalog, which will still include the old apiGroup for the time being. I fixed the one in the contribution guide as well. All other entries look fine to me. Could you please re-check?

pavolloffay · 2019-03-06T08:44:05Z

I fixed the OLM catalog, which will still include the old apiGroup for the time being.

We should maybe create an issue to migrate it to the new one?

pavolloffay

LGTM, I would move freeform.go and options.go to a different package so it can be shared for both impls - to avoid doplicating two impls

jpkrohling · 2019-03-06T08:51:45Z

We should maybe create an issue to migrate it to the new one?

I meant, it has both apiGroups now.

Moved from v1alpha1 to v1

785daf1

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling requested a review from objectiser March 5, 2019 15:32

Added v1alpha1 back with a warning to use the new apiGroup

c332e90

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

Added new api to .codecov ignore

88b46e3

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling force-pushed the 201-Reverse-apiGroup branch from b5a0c7e to 88b46e3 Compare March 5, 2019 16:03

jpkrohling requested a review from pavolloffay March 5, 2019 16:03

This was referenced Mar 5, 2019

Split the E2E tests via tags #266

Closed

Add rollover support #267

Merged

objectiser approved these changes Mar 5, 2019

View reviewed changes

Fixes based on the review

9e07630

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

pavolloffay reviewed Mar 6, 2019

View reviewed changes

CONTRIBUTING.adoc Show resolved Hide resolved

deploy/crds/jaegertracing_v1_jaeger_cr.yaml Show resolved Hide resolved

deploy/crds/jaegertracing_v1_jaeger_crd.yaml Show resolved Hide resolved

deploy/crds/jaegertracing_v1_jaeger_crd.yaml Outdated Show resolved Hide resolved

jpkrohling added 2 commits March 6, 2019 09:13

Updated olm-catalog files and cleaned CRD

d1113a1

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

Fixed case where legacy CRD isn't installed

9d48000

Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>

jpkrohling requested a review from pavolloffay March 6, 2019 08:28

pavolloffay approved these changes Mar 6, 2019

View reviewed changes

jpkrohling merged commit fcb26f4 into jaegertracing:master Mar 6, 2019

jpkrohling mentioned this pull request Mar 6, 2019

Reverse io.jaegertracing apiGroup #201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved from v1alpha1 to v1 #265

Moved from v1alpha1 to v1 #265

jpkrohling commented Mar 5, 2019

jpkrohling commented Mar 5, 2019

jpkrohling commented Mar 5, 2019

codecov bot commented Mar 5, 2019 •

edited

Loading

jpkrohling commented Mar 5, 2019

objectiser left a comment

objectiser Mar 5, 2019

objectiser Mar 5, 2019

jpkrohling commented Mar 6, 2019

pavolloffay left a comment

jpkrohling commented Mar 6, 2019

pavolloffay commented Mar 6, 2019

pavolloffay left a comment

jpkrohling commented Mar 6, 2019

Moved from v1alpha1 to v1 #265

Moved from v1alpha1 to v1 #265

Conversation

jpkrohling commented Mar 5, 2019

jpkrohling commented Mar 5, 2019

jpkrohling commented Mar 5, 2019

codecov bot commented Mar 5, 2019 • edited Loading

Codecov Report

jpkrohling commented Mar 5, 2019

objectiser left a comment

Choose a reason for hiding this comment

objectiser Mar 5, 2019

Choose a reason for hiding this comment

objectiser Mar 5, 2019

Choose a reason for hiding this comment

jpkrohling commented Mar 6, 2019

pavolloffay left a comment

Choose a reason for hiding this comment

jpkrohling commented Mar 6, 2019

pavolloffay commented Mar 6, 2019

pavolloffay left a comment

Choose a reason for hiding this comment

jpkrohling commented Mar 6, 2019

codecov bot commented Mar 5, 2019 •

edited

Loading