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.

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

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

Merged
merged 3 commits into from Sep 20, 2023

Conversation

semihbkgr
Copy link
Member

Description

The configuration for persistence of SQL metadata shouldn't be changed once it was enabled

@semihbkgr semihbkgr requested a review from a team as a code owner September 20, 2023 08:37
@semihbkgr semihbkgr requested review from SeriyBg and dzeromski-hazelcast and removed request for a team September 20, 2023 08:38
@@ -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

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

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

@semihbkgr semihbkgr temporarily deployed to report September 20, 2023 09:59 — with GitHub Actions Inactive
@github-actions
Copy link

Test Results

Total Tests 🔴 Failures 🟠 Errors ⚪ Skipped
OS 101 34 0 52
EE 103 51 0 29
Failed Tests
OS
  • Hazelcast Map Config should create Map Config with correct default values
  • Hazelcast Queue Config should create Queue Config with correct default values
  • Resilience should be able to reconnect to Hazelcast cluster upon restart even when Hazelcast cluster is marked to be deleted
  • Hazelcast Cache Config should create Cache Config
  • Hazelcast Cache Config should create Cache Config with correct default values
  • Hazelcast JetJob should change JetJob status
  • Hazelcast User Code Deployment should use the MapStore implementation correctly using user code from bucket
  • Hazelcast Map Config should update the map correctly
  • Hazelcast User Code Deployment should use the MapStore implementation correctly using user code from remote url
  • Hazelcast External API errors should be reflected to Hazelcast CR status
  • Hazelcast Queue Config should fail to update Queue Config
  • Hazelcast Map Config when Native Memory is not enabled for Hazelcast CR should fail with InMemoryFormat value is set to NativeMemory
  • Hazelcast Map Config when Native Memory is not enabled for Hazelcast CR should fail with InMemoryFormat value is set to NativeMemory in near cache configuration
  • Hazelcast JetJob should execute JetJob successfully using jar from bucket
  • Hazelcast ReplicatedMap Config should fail to update ReplicatedMap Config
  • Hazelcast JetJob should execute JetJob successfully using jar from remote url
  • Hazelcast Cache Config when Native Memory is not enabled for Hazelcast CR should fail with InMemoryFormat value is set to NativeMemory
  • Hazelcast ReplicatedMap Config should create ReplicatedMap Config
  • Hazelcast ReplicatedMap Config should create ReplicatedMap Config with correct default values
  • Hazelcast User Code Deployment should add entry listeners
  • Hazelcast MultiMap Config should fail to update MultiMap Config
  • Hazelcast Map Config with Persistence should fail when persistence of Map CR and Hazelcast CR do not match
  • Hazelcast MultiMap Config should create MultiMap Config
  • Hazelcast MultiMap Config should create MultiMap Config with correct default values
  • Hazelcast Topic Config should create Topic Config with correct default values
  • Hazelcast Map Config should create Map Config
  • Management-Center ManagementCenter CR without Persistence Should create ManagementCenter resources and no PVC
  • Hazelcast Hazelcast member status should update HZ detailed member status
  • Hazelcast Map Config should create Map Config with Indexes
  • Hazelcast Map Config with Persistence should continue persisting last applied Map Config in case of failure
  • Hazelcast Map Config with Persistence should persist Map Config with Indexes
  • Hazelcast Topic Config should fail to update Topic Config
  • Hazelcast Queue Config should create Queue Config
  • Hazelcast JetJob should fail the job if Hz cluster is failing
  • EE
  • Hazelcast Cache Config should create Cache Config
  • Hazelcast JetJob should change JetJob status
  • Hazelcast CR with Persistence feature enabled Should successfully restore from ExternalBackup-PVC using GCP bucket HotBackupResourceName
  • Hazelcast Cache Config should create Cache Config with correct default values
  • Hazelcast Map Config should create Map Config with correct default values
  • Hazelcast CR with Persistence feature enabled Should successfully restore from ExternalBackup-PVC using AWS S3 bucket HotBackupResourceName
  • Hazelcast User Code Deployment should use the MapStore implementation correctly using user code from bucket
  • Hazelcast User Code Deployment should use the MapStore implementation correctly using user code from remote url
  • Hazelcast Map Config when Native Memory is not enabled for Hazelcast CR should fail with InMemoryFormat value is set to NativeMemory
  • Hazelcast WAN should send data to another cluster
  • Hazelcast Queue Config should fail to update Queue Config
  • Hazelcast Map Config should update the map correctly
  • Hazelcast Map Config when Native Memory is not enabled for Hazelcast CR should fail with InMemoryFormat value is set to NativeMemory in near cache configuration
  • Hazelcast ReplicatedMap Config should fail to update ReplicatedMap Config
  • Hazelcast Cache Config when Native Memory is not enabled for Hazelcast CR should fail with InMemoryFormat value is set to NativeMemory
  • Hazelcast JetJobSnapshot should export a snapshot canceling the job
  • Hazelcast User Code Deployment should add executor services both initially and dynamically
  • Hazelcast WAN should send data using WanReplication configuration in Config
  • Hazelcast CR with Persistence feature enabled should trigger corresponding action when startupActionSpecified ForceStart
  • Hazelcast WAN should fail first and after spec removal, should succeed
  • Hazelcast JetJobSnapshot should export snapshot and initialize new job from that snapshot
  • Hazelcast CR with Persistence feature enabled should trigger corresponding action when startupActionSpecified PartialStart
  • Hazelcast ReplicatedMap Config should create ReplicatedMap Config
  • Hazelcast ReplicatedMap Config should create ReplicatedMap Config with correct default values
  • Hazelcast MultiMap Config should fail to update MultiMap Config
  • Hazelcast JetJobSnapshot should set status to 'failed' if exporting snapshot from non-running job
  • Hazelcast CR with Persistence feature enabled should successfully restore from LocalBackup-PVC-HotBackupResourceName
  • Hazelcast WAN should stop replicating to target cluster
  • Hazelcast Map Config with Persistence should keep the entries after a Hot Backup
  • Hazelcast Hazelcast CR dependent CRs when Hazelcast CR is deleted dependent Data Structures and HotBackup CRs should be deleted
  • Hazelcast MultiMap Config should create MultiMap Config with correct default values
  • Hazelcast WAN should delete the map from status and WAN status should be Pending
  • Hazelcast Cache Config with Persistence should keep the entries after a Hot Backup
  • Hazelcast MultiMap Config should create MultiMap Config
  • Hazelcast Topic Config should create Topic Config with correct default values
  • Hazelcast Map Config with Persistence should persist the map successfully created configs into the config
  • Hazelcast WAN should start WanReplication for the maps created after the Wan CR
  • Hazelcast Topic Config should create Topic Config
  • Hazelcast Hazelcast member status should update HZ detailed member status
  • Hazelcast Map Config should create Map Config
  • Hazelcast WAN should continue replication if one reference is deleted
  • Hazelcast Map Config with Persistence should continue persisting last applied Map Config in case of failure
  • Hazelcast Map Config should create Map Config with Indexes
  • Hazelcast Cache Config with Persistence should persist the cache successfully created configs into the config
  • Hazelcast Backup should fail if bucket credential of external backup in secret is not correct
  • Hazelcast Map Config with Persistence should persist Map Config with Indexes
  • Hazelcast WAN should replicate maps to target cluster including maps with names different from map CR name
  • Hazelcast Topic Config should fail to update Topic Config
  • Hazelcast WAN should delete the map from status and WAN status should be failed
  • Hazelcast Queue Config should create Queue Config
  • Hazelcast JetJob should fail the job if Hz cluster is failing
  • @semihbkgr semihbkgr temporarily deployed to report September 20, 2023 13:24 — with GitHub Actions Inactive
    @semihbkgr semihbkgr added the fix This type is used to identify changes related to backward-compatible bug fixes label Sep 20, 2023
    @semihbkgr semihbkgr added this to the 5.10 milestone Sep 20, 2023
    @semihbkgr semihbkgr merged commit 1c948ce into main Sep 20, 2023
    50 checks passed
    @semihbkgr semihbkgr deleted the cn-983 branch September 20, 2023 13:50
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    fix This type is used to identify changes related to backward-compatible bug fixes
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    3 participants