Updated Operator SDK to v0.18.2 #1126

jpkrohling · 2020-07-09T11:17:18Z

Signed-off-by: Juraci Paixão Kröhling [email protected]

pkg/ingress/ingress_test.go

codecov · 2020-07-09T11:25:46Z

Codecov Report

Merging #1126 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1126   +/-   ##
=======================================
  Coverage   88.22%   88.22%           
=======================================
  Files          86       86           
  Lines        5350     5351    +1     
=======================================
+ Hits         4720     4721    +1     
  Misses        466      466           
  Partials      164      164

Impacted Files	Coverage Δ
pkg/apis/jaegertracing/v1/jaeger_types.go	`100.00% <ø> (ø)`
pkg/service/collector.go	`100.00% <ø> (ø)`
pkg/autodetect/main.go	`82.45% <100.00%> (+0.10%)`	⬆️

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 89dfa03...f401f16. Read the comment docs.

jpkrohling · 2020-07-09T14:13:37Z

Apparently, the coverage change is related to the skipping of the ingress test.

jpkrohling · 2020-07-09T14:16:24Z

Makefile

@@ -259,7 +259,7 @@ kafka: deploy-kafka-operator
 	@kubectl create namespace $(KAFKA_NAMESPACE) 2>&1 | grep -v "already exists" || true
 	@curl --fail --location $(KAFKA_EXAMPLE) --output deploy/test/kafka-example.yaml --create-dirs
 	@sed -i 's/size: 100Gi/size: 10Gi/g' deploy/test/kafka-example.yaml
-	@kubectl -n $(KAFKA_NAMESPACE) apply --dry-run=client -f deploy/test/kafka-example.yaml
+	@kubectl -n $(KAFKA_NAMESPACE) apply --dry-run=true -f deploy/test/kafka-example.yaml


@kevinearls I think you added this flag in the past. Not sure where this option is parsed, but it currently fails with my client/server combination. I got it working by setting it to client, but wanted to check with you whether this is indeed the correct value.

For reference, here are the versions I'm using:

Client Version: version.Info{Major:"1", Minor:"17", GitVersion:"v1.17.0-4-g38212b5", GitCommit:"d038424d6d4f1cc39ad586ac0d36dac3a7a37ceb", GitTreeState:"clean", BuildDate:"2020-06-16T03:29:46Z", GoVersion:"go1.13.4", Compiler:"gc", Platform:"linux/amd64"} Server Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.3", GitCommit:"2e7996e3e2712684bc73f0dec0200d64eec7fe40", GitTreeState:"clean", BuildDate:"2020-05-20T12:43:34Z", GoVersion:"go1.13.9", Compiler:"gc", Platform:"linux/amd64"}

jpkrohling · 2020-07-09T14:17:35Z

deploy/cluster_role.yaml

-  - 'update'
-  - 'delete'
-  - 'watch'
+  - create


This change is mainly to reduce the diff between new operators and this operator, in an attempt to make it easier to compare what changed between SDK versions.

The new list contains patch, which the old list didn't. Apart from that, it's only ordering and formatting changes.

jpkrohling · 2020-07-09T14:19:49Z

test/e2e/all_in_one_test.go

@@ -41,8 +41,8 @@ func (suite *AllInOneTestSuite) SetupSuite() {
 		require.FailNow(t, "Failed in prepare")
 	}
 	fw = framework.Global
-	namespace, _ = ctx.GetNamespace()


@kevinearls heads up: GetNamespace actually creates a new namespace (!!), which fails during runtime as the namespace already exists. Behind the scenes, GetNamespace calls GetID to obtain the namespace's value, hence this change.

jpkrohling · 2020-07-09T14:23:27Z

test/e2e/utils.go

 	osv1 "github.com/openshift/api/route/v1"
 	osv1sec "github.com/openshift/api/security/v1"
 	framework "github.com/operator-framework/operator-sdk/pkg/test"
 	"github.com/operator-framework/operator-sdk/pkg/test/e2eutil"
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	"golang.org/x/net/context"


This seems to be added via 5c37d2d , but I guess this was supposed to be context instead. Just wanted to confirm with you if you really meant this old library, or if we can safely change it to use the stdlib's context.

kevinearls · 2020-07-10T09:40:11Z

@jpkrohling I am getting a failure running E2E tests on an OCP cluster, although strangely it's not 100% clear what is failing or why. I will work on this more later today.

objectiser

LGTM - just a couple of clarifications.

deploy/cluster_role.yaml

deploy/olm-catalog/jaeger-operator/1.18.1/jaeger-operator.v1.18.1.clusterserviceversion.yaml

objectiser · 2020-07-10T10:46:36Z

pkg/apis/jaegertracing/v1/jaeger_types.go

@@ -153,11 +153,11 @@ type Jaeger struct {
 // +k8s:openapi-gen=true
 type JaegerCommonSpec struct {
 	// +optional
-	// +listType=set
+	// +listType=atomic


Why these changes?

There was a change in how OpenAPI and the generated resources would validate the values. I don't remember exactly the reason, as this is something I faced during the upgrade of the OpenTelemetry Operator as well: https://github.com/open-telemetry/opentelemetry-operator/pull/24/files#diff-0f1140984003d7bcbaddb3717895a48d

pavolloffay

LGTM just some nits.

Does this PR pass the full e2e tests including e2e-tests-self-provisioned-es on OCP?

go.mod

pavolloffay · 2020-07-10T11:09:21Z

pkg/autodetect/main.go

@@ -197,6 +197,7 @@ func (b *Background) detectKafka(ctx context.Context, apiList *metav1.APIGroupLi

 func (b *Background) detectClusterRoles(ctx context.Context) {
 	tr := &authenticationapi.TokenReview{
+		ObjectMeta: metav1.ObjectMeta{Name: "jaeger-operator-TEST"},


No, this is actually required now, otherwise it fails to validate ("token review has to have a name" or something...)

Could this cause any issues when there are multiple operands in the same namespace or multiple operators in the cluster?

pkg/cmd/start/main.go

pkg/service/collector.go

kevinearls · 2020-07-10T11:16:45Z

I found a few problems with tests:

W0710 09:17:42.229260   17185 helpers.go:549] --dry-run=true is deprecated (boolean value) and can be replaced with --dry-run=client.
# github.com/jaegertracing/jaeger-operator/test/e2e [github.com/jaegertracing/jaeger-operator/test/e2e.test]
test/e2e/self_provisioned_elasticsearch_test.go:143:72: not enough arguments in call to fw.KubeClient.AppsV1().Deployments(namespace).List
	have ("k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
	want (context.Context, "k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
test/e2e/self_provisioned_elasticsearch_test.go:162:59: not enough arguments in call to fw.KubeClient.CoreV1().Pods(namespace).List
	have ("k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
	want (context.Context, "k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
test/e2e/self_provisioned_elasticsearch_test.go:258:67: not enough arguments in call to kubeclient.AppsV1().Deployments(namespace).Get
	have (string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
	want (context.Context, string, "k8s.io/apimachinery/pkg/apis/meta/v1".GetOptions)
make: *** [e2e-tests-self-provisioned-es] Error 2

Fixes jaegertracing#1125 Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Signed-off-by: Kevin Earls <[email protected]>

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling · 2020-07-10T11:53:06Z

@pavolloffay said:

Does this PR pass the full e2e tests including e2e-tests-self-provisioned-es on OCP?

I think those are the ones that @kevinearls mentioned a few minutes ago and fixed in his branch. I cherry-picked his commit and added to this PR.

I also addressed the other comments. The only open comment is about the change from set to atomic (@objectiser). If more details about is needed, I can revert that and trace back why it's needed.

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

kevinearls · 2020-07-10T12:43:48Z

@jpkrohling @pavolloffay I did a rerun on an OCP 4.4 cluster and all tests have passed, including the self provisioned es tests.

kevinearls

LGTM, tests are now passing on OCP 4.4

chlunde · 2020-07-16T19:33:25Z

test/e2e/all_in_one_test.go

-	namespace, _ = ctx.GetNamespace()
-	require.NotNil(t, namespace, "GetNamespace failed")
+	namespace = ctx.GetID()
+	require.NotNil(t, namespace, "GetID failed")


Should these be NotEmpty now? But I'm not sure it can fail at this point

Makefile

jpkrohling requested a review from objectiser July 9, 2020 11:17

jpkrohling commented Jul 9, 2020

View reviewed changes

pkg/ingress/ingress_test.go Outdated Show resolved Hide resolved

jpkrohling force-pushed the jpkrohling/issue1125 branch from 1e03bf0 to 676c3b7 Compare July 9, 2020 11:21

jpkrohling force-pushed the jpkrohling/issue1125 branch 3 times, most recently from afbda92 to c56ce19 Compare July 9, 2020 13:33

jpkrohling commented Jul 9, 2020

View reviewed changes

jpkrohling requested review from kevinearls and removed request for objectiser July 9, 2020 14:24

objectiser reviewed Jul 10, 2020

View reviewed changes

pavolloffay reviewed Jul 10, 2020

View reviewed changes

jpkrohling mentioned this pull request Jul 10, 2020

Permissions for apps/deployments is repeated #1130

Closed

jpkrohling force-pushed the jpkrohling/issue1125 branch from 4f04da8 to ef1e48e Compare July 10, 2020 11:44

jpkrohling and others added 7 commits July 10, 2020 13:51

Updated Operator SDK to v0.18.2

76c22b4

Fixes jaegertracing#1125 Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Fixes for the e2e tests

fc0819b

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Use operator-sdk run local instead of custom runner

51acf6a

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Enable the ingress test back

ae52473

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Fixes based on the review

6d7db6d

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

Fix compile errors by adding context parameter

8180d40

Signed-off-by: Kevin Earls <[email protected]>

Rebased and added context to Events call

de8e716

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

jpkrohling force-pushed the jpkrohling/issue1125 branch from ef1e48e to de8e716 Compare July 10, 2020 11:52

Compile the tests without running them as part of 'build'

f401f16

Signed-off-by: Juraci Paixão Kröhling <[email protected]>

kevinearls approved these changes Jul 10, 2020

View reviewed changes

pavolloffay approved these changes Jul 10, 2020

View reviewed changes

objectiser approved these changes Jul 10, 2020

View reviewed changes

jpkrohling merged commit bd60752 into jaegertracing:master Jul 10, 2020

chlunde reviewed Jul 16, 2020

View reviewed changes

Makefile Show resolved Hide resolved

chlunde mentioned this pull request Jul 17, 2020

Update downloaded SDK version and update deprecated struct name #1133

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated Operator SDK to v0.18.2 #1126

Updated Operator SDK to v0.18.2 #1126

jpkrohling commented Jul 9, 2020

codecov bot commented Jul 9, 2020 •

edited

Loading

jpkrohling commented Jul 9, 2020

jpkrohling Jul 9, 2020

jpkrohling Jul 9, 2020

jpkrohling Jul 9, 2020

jpkrohling Jul 9, 2020

kevinearls commented Jul 10, 2020

objectiser left a comment

objectiser Jul 10, 2020

jpkrohling Jul 10, 2020

objectiser Jul 10, 2020

pavolloffay left a comment

pavolloffay Jul 10, 2020

jpkrohling Jul 10, 2020

pavolloffay Jul 10, 2020 •

edited

Loading

kevinearls commented Jul 10, 2020 •

edited by jpkrohling

Loading

jpkrohling commented Jul 10, 2020

kevinearls commented Jul 10, 2020

kevinearls left a comment

chlunde Jul 16, 2020

Updated Operator SDK to v0.18.2 #1126

Updated Operator SDK to v0.18.2 #1126

Conversation

jpkrohling commented Jul 9, 2020

codecov bot commented Jul 9, 2020 • edited Loading

Codecov Report

jpkrohling commented Jul 9, 2020

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

kevinearls commented Jul 10, 2020

objectiser 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

pavolloffay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

kevinearls commented Jul 10, 2020 • edited by jpkrohling Loading

jpkrohling commented Jul 10, 2020

kevinearls commented Jul 10, 2020

kevinearls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 9, 2020 •

edited

Loading

pavolloffay Jul 10, 2020 •

edited

Loading

kevinearls commented Jul 10, 2020 •

edited by jpkrohling

Loading