Add support for imagePullSecrets for Scylla and Agent images #678

rzetelskik · 2021-07-05T13:56:20Z

Description of your changes:

Add support for imagePullSecrets for Scylla and Agent images.
Add unit tests for StatefulSetForRack, including imagePullSecrets diffs

Resolves #348

zimnx

Thanks, please rename PR title and commit msg to follow current naming convention.
Please also extract autogenerated changes to separate commit.

pkg/api/scylla/v1/cluster_types.go

pkg/controller/scyllacluster/resource/resource_test.go

pkg/api/scylla/v1/cluster_types.go

zimnx · 2021-07-05T17:41:42Z

pkg/controller/scyllacluster/resource/resource_test.go

+
+	storageCapacity, _ := resource.ParseQuantity(basicRack.Storage.Capacity)
+
+	expectedSts := &appsv1.StatefulSet{


oh this is painful to work with. Do something simillar to this approach:

scylla-operator/pkg/resourceapply/apps_test.go

Lines 145 to 150 in 9c745f7

expectedSts: func() *appsv1.StatefulSet {

sts := newSts()

sts.Spec.Replicas = pointer.Int32Ptr(*sts.Spec.Replicas + 1)

utilruntime.Must(SetHashAnnotation(sts))

return sts

}(),

pkg/controller/scyllacluster/resource/resource_test.go

pkg/api/scylla/v1/cluster_types.go

pkg/controller/scyllacluster/resource/resource_test.go

tnozicka · 2021-07-21T06:21:11Z

API approved

pkg/controller/scyllacluster/resource/resource_test.go

tnozicka · 2021-07-23T12:37:57Z

pkg/controller/scyllacluster/resource/resource_test.go

+		}
+	}
+
+	newNonNilImagePullSecrets := func() []corev1.LocalObjectReference {


nit: I'd find it more readable we inline short functions like that, but that might be opinionated. in the other case the nonNil prefix is kind of implicit, I'd not assume you make a function to create nil

I was taking example from how it's done in existing test, e.g. cluster_validation_test. Inlining would require duplicating the anonymous function in each test case. What would you change the "nonNil" to?

I was taking example from how it's done in existing test, e.g. cluster_validation_test.

I guess that could have used some tweaking as well :)

Inlining would require duplicating the anonymous function in each test case.

actually, there wouldn't be an anonymous function at all

sc.Spec.ImagePullSecrets = []corev1.LocalObjectReference{ { Name: "basic-secrets", }, }

What would you change the "nonNil" to?

I'd drop it, the other helper functions (like newBasicStatefulSet) don't start with the prefix, yet we assume they return non-nil values

Thanks, I changed the shorter functions to inline and renamed the other ones

tnozicka · 2021-07-23T12:39:26Z

pkg/controller/scyllacluster/resource/resource_test.go

+			expectedStatefulSet: nil,
+			expectedError: func() error {
+				_, err := resource.ParseQuantity("")
+				return fmt.Errorf("cannot parse %q: %v", "", err)


nit: I'd just put the string inside, calling the function assumes to much internals that could change while the string stays constant

compared string + added cmpopts.EquateErrors() for recommended error comparison

pkg/controller/scyllacluster/resource/resource_test.go

tnozicka

please split generated files and go.mod + vendor into separate commits

tnozicka · 2021-07-27T10:50:47Z

pkg/controller/scyllacluster/resource/resource_test.go

+		}
+	}
+
+	newNonNilImagePullSecrets := func() []corev1.LocalObjectReference {


I was taking example from how it's done in existing test, e.g. cluster_validation_test.

I guess that could have used some tweaking as well :)

Inlining would require duplicating the anonymous function in each test case.

actually, there wouldn't be an anonymous function at all

sc.Spec.ImagePullSecrets = []corev1.LocalObjectReference{ { Name: "basic-secrets", }, }

What would you change the "nonNil" to?

I'd drop it, the other helper functions (like newBasicStatefulSet) don't start with the prefix, yet we assume they return non-nil values

pkg/controller/scyllacluster/resource/resource_test.go

tnozicka

nits, lgtm otherwise

pkg/controller/scyllacluster/resource/resource_test.go

tnozicka

/lgtm
thanks!

zimnx

LGTM, thanks

rzetelskik requested a review from zimnx July 5, 2021 13:56

rzetelskik added the kind/feature Categorizes issue or PR as related to a new feature. label Jul 5, 2021

rzetelskik added this to the v1.4 milestone Jul 5, 2021

rzetelskik force-pushed the private_scylla_images branch from 37c0923 to 43c60cf Compare July 5, 2021 14:01

zimnx reviewed Jul 5, 2021

View reviewed changes

pkg/api/scylla/v1/cluster_types.go Outdated Show resolved Hide resolved

pkg/controller/scyllacluster/resource/resource_test.go Outdated Show resolved Hide resolved

rzetelskik force-pushed the private_scylla_images branch 2 times, most recently from d4569cc to 275fb05 Compare July 5, 2021 15:39

rzetelskik changed the title ~~api: add ImagePullSecrets - fixes #348~~ Add support for imagePullSecrets for Scylla and Agent images Jul 5, 2021

rzetelskik requested a review from zimnx July 5, 2021 15:45

zimnx reviewed Jul 5, 2021

View reviewed changes

rzetelskik force-pushed the private_scylla_images branch from 275fb05 to 6cb9b3d Compare July 6, 2021 10:49

rzetelskik requested a review from zimnx July 6, 2021 11:26

rzetelskik force-pushed the private_scylla_images branch from 2c6c296 to bac5dd2 Compare July 6, 2021 13:58

tnozicka reviewed Jul 7, 2021

View reviewed changes

rzetelskik force-pushed the private_scylla_images branch 2 times, most recently from b333f18 to 0b19dd8 Compare July 7, 2021 16:17

rzetelskik requested a review from tnozicka July 7, 2021 16:19

rzetelskik force-pushed the private_scylla_images branch from edc5715 to fe00fe5 Compare July 7, 2021 21:35

tnozicka modified the milestones: v1.4, v1.5 Jul 8, 2021

rzetelskik force-pushed the private_scylla_images branch 3 times, most recently from 8023196 to 73e8622 Compare July 13, 2021 10:41

tnozicka requested changes Jul 21, 2021

View reviewed changes

pkg/controller/scyllacluster/resource/resource_test.go Outdated Show resolved Hide resolved

pkg/controller/scyllacluster/resource/resource_test.go Outdated Show resolved Hide resolved

tnozicka added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jul 21, 2021

rzetelskik force-pushed the private_scylla_images branch 2 times, most recently from 63486a4 to ad8f159 Compare July 21, 2021 10:10

tnozicka reviewed Jul 21, 2021

View reviewed changes

pkg/controller/scyllacluster/resource/resource_test.go Outdated Show resolved Hide resolved

tnozicka reviewed Jul 21, 2021

View reviewed changes

pkg/controller/scyllacluster/resource/resource_test.go Outdated Show resolved Hide resolved

rzetelskik force-pushed the private_scylla_images branch from ad8f159 to 5e45575 Compare July 21, 2021 11:51

rzetelskik requested a review from tnozicka July 21, 2021 12:03

rzetelskik force-pushed the private_scylla_images branch from 5e45575 to 1830718 Compare July 23, 2021 08:36

tnozicka reviewed Jul 23, 2021

View reviewed changes

rzetelskik force-pushed the private_scylla_images branch 2 times, most recently from a0ccc09 to 43c26f8 Compare July 24, 2021 11:12

rzetelskik requested a review from tnozicka July 24, 2021 12:26

tnozicka reviewed Jul 27, 2021

View reviewed changes

rzetelskik force-pushed the private_scylla_images branch 2 times, most recently from 08a7d79 to 441f53f Compare July 28, 2021 08:24

rzetelskik requested a review from tnozicka July 28, 2021 08:27

tnozicka reviewed Jul 28, 2021

View reviewed changes

pkg/controller/scyllacluster/resource/resource_test.go Outdated Show resolved Hide resolved

pkg/controller/scyllacluster/resource/resource_test.go Show resolved Hide resolved

rzetelskik force-pushed the private_scylla_images branch from 441f53f to 150697e Compare July 28, 2021 12:40

rzetelskik added 3 commits July 28, 2021 14:56

Add support for imagePullSecrets for Scylla and Agent images

03a32dd

Update go.mod, go.sum and vendor

db9abbe

Update autogenerated files

34f4411

rzetelskik force-pushed the private_scylla_images branch from 150697e to 34f4411 Compare July 28, 2021 12:57

tnozicka approved these changes Jul 28, 2021

View reviewed changes

zimnx approved these changes Jul 29, 2021

View reviewed changes

zimnx merged commit 9230a4a into scylladb:master Jul 29, 2021

rzetelskik deleted the private_scylla_images branch July 29, 2021 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for imagePullSecrets for Scylla and Agent images #678

Add support for imagePullSecrets for Scylla and Agent images #678

rzetelskik commented Jul 5, 2021 •

edited

zimnx left a comment

zimnx Jul 5, 2021

rzetelskik Jul 6, 2021

tnozicka commented Jul 21, 2021

tnozicka Jul 23, 2021

rzetelskik Jul 24, 2021

tnozicka Jul 27, 2021

rzetelskik Jul 28, 2021

tnozicka Jul 23, 2021

rzetelskik Jul 24, 2021

tnozicka left a comment

tnozicka Jul 27, 2021

tnozicka left a comment

tnozicka left a comment

zimnx left a comment


		storageCapacity, _ := resource.ParseQuantity(basicRack.Storage.Capacity)

		expectedSts := &appsv1.StatefulSet{

	expectedSts: func() *appsv1.StatefulSet {
	sts := newSts()
	sts.Spec.Replicas = pointer.Int32Ptr(*sts.Spec.Replicas + 1)
	utilruntime.Must(SetHashAnnotation(sts))
	return sts
	}(),

Add support for imagePullSecrets for Scylla and Agent images #678

Add support for imagePullSecrets for Scylla and Agent images #678

Conversation

rzetelskik commented Jul 5, 2021 • edited

zimnx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka commented Jul 21, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka left a comment

Choose a reason for hiding this comment

tnozicka left a comment

Choose a reason for hiding this comment

zimnx left a comment

Choose a reason for hiding this comment

rzetelskik commented Jul 5, 2021 •

edited