Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CN-983] Persistence of SQL metadata shouldn't be changed once it was enabled #883

Merged
merged 3 commits into from Sep 20, 2023
Merged

[CN-983] Persistence of SQL metadata shouldn't be changed once it was enabled #883

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c09452b
validation for sql catalogPersistenceEnabled
semihbkgr Sep 20, 2023
d3b1402
update error path
semihbkgr Sep 20, 2023
c36d691
fix validation
semihbkgr Sep 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions api/v1alpha1/hazelcast_validation.go
View file
Open in desktop
Expand Up @@ -360,6 +360,7 @@ func (v *hazelcastValidator) validateNotUpdatableHazelcastFields(current *Hazelc
}

v.validateNotUpdatableHzPersistenceFields(current.Persistence, last.Persistence)
v.validateNotUpdatableSQLFields(current.SQL, last.SQL)
}

func (v *hazelcastValidator) validateNotUpdatableHzPersistenceFields(current, last *HazelcastPersistenceConfiguration) {
Expand All @@ -386,6 +387,12 @@ func (v *hazelcastValidator) validateNotUpdatableHzPersistenceFields(current, la
}
}

func (v *hazelcastValidator) validateNotUpdatableSQLFields(current, last *SQL) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest using simpler name like: validateSQLFields.

Copy link
Member Author

@semihbkgr semihbkgr Sep 20, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the convention we use in the validation.
e.g:

  • validateNotUpdatableHzPersistenceFields
  • validateNotUpdatableHazelcastFields

if last.CatalogPersistenceEnabled && !current.CatalogPersistenceEnabled {
v.Forbidden(Path("spec", "catalogPersistenceEnabled"), "field cannot be disabled after it has been enabled")
semihbkgr marked this conversation as resolved.
Show resolved Hide resolved
}
}

func (v *hazelcastValidator) validateJetConfig(h *Hazelcast) {
j := h.Spec.JetEngineConfiguration
p := h.Spec.Persistence
Expand Down
30 changes: 30 additions & 0 deletions test/integration/hazelcast_test.go
View file
Open in desktop
Expand Up @@ -2185,6 +2185,36 @@ var _ = Describe("Hazelcast CR", func() {

Expect(hz.Spec.SQL.CatalogPersistenceEnabled).Should(BeTrue())
})

It("should fail to disable catalogPersistence", Label("fast"), func() {
spec := test.HazelcastSpec(defaultHazelcastSpecValues(), ee)
spec.Persistence = &hazelcastv1alpha1.HazelcastPersistenceConfiguration{
BaseDir: "/data/hot-restart/",
ClusterDataRecoveryPolicy: hazelcastv1alpha1.FullRecovery,
Pvc: &hazelcastv1alpha1.PersistencePvcConfiguration{
AccessModes: []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce},
RequestStorage: resource.NewQuantity(9*2^20, resource.BinarySI),
},
}
spec.SQL = &hazelcastv1alpha1.SQL{
CatalogPersistenceEnabled: true,
}

hzSpec, _ := json.Marshal(&spec)

hz := &hazelcastv1alpha1.Hazelcast{
ObjectMeta: randomObjectMeta(namespace, n.LastSuccessfulSpecAnnotation, string(hzSpec)),
Spec: spec,
}

Create(hz)
hz = ensureHzStatusIsPending(hz)
Expect(hz.Spec.SQL.CatalogPersistenceEnabled).Should(BeTrue())

hz.Spec.SQL.CatalogPersistenceEnabled = false
err := k8sClient.Update(context.Background(), hz)
Expect(err).Should(MatchError(ContainSubstring("field cannot be disabled after it has been enabled")))
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think we need to check for exact match of error message. I dont think we do it in other tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not? Ideally we should do it other tests too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dzeromski-hazelcast we do it in other tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that putting such a specific check in test is unnecessary and makes maintaining tests harder in the long run. But up to you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would disagree. Errors are part of the public API, so in this case, we make sure to test the public API and not have a false-positive test, in case we get the err but is not related to the validation

})
})
})