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-902] change WanReplication validation #803

Merged
merged 6 commits into from Jul 12, 2023
Merged

[CN-902] change WanReplication validation #803

merged 6 commits into from Jul 12, 2023

Conversation

SeriyBg
Copy link
Contributor

@SeriyBg SeriyBg commented Jul 11, 2023

User Impact

Added webhook validation for WanReplication update and changed the error messages to be consistent with other validations

@SeriyBg SeriyBg requested a review from a team as a code owner July 11, 2023 08:57
@SeriyBg SeriyBg requested review from semihbkgr and dzeromski-hazelcast and removed request for a team July 11, 2023 08:57
@SeriyBg SeriyBg added the enhancement New feature or request label Jul 11, 2023
@SeriyBg SeriyBg added this to the 5.9 milestone Jul 11, 2023
semihbkgr
semihbkgr approved these changes Jul 11, 2023
View reviewed changes
Copy link
Member

@semihbkgr semihbkgr left a comment

Choose a reason for hiding this comment

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

LGTM 👍 with some refactoring.

controllers/hazelcast/wanreplication_controller.go Outdated

err = validateNotUpdatableFields(&wan.Spec, lastSpec)
if err != nil {
if _, successfullyAppliedBefore := wan.ObjectMeta.Annotations[n.LastSuccessfulSpecAnnotation]; successfullyAppliedBefore {
Copy link
Member

Choose a reason for hiding this comment

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

Is it really required to check if LastSuccessfulSpecAnnotation exists in annotations? validateNonUpdatableWanFields function already checks it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Removed it

@SeriyBg SeriyBg temporarily deployed to report July 11, 2023 09:36 — with GitHub Actions Inactive
@github-actions
Copy link

Test Results

Total Tests 🔴 Failures 🟠 Errors ⚪ Skipped
OS 92 0 0 45
EE 92 1 0 14
Failed Tests
OS
EE
  • Hazelcast CR with Persistence feature enabled should trigger corresponding action when startupActionSpecified ForceStart
    		•

    @SeriyBg SeriyBg temporarily deployed to report July 11, 2023 11:09 — with GitHub Actions Inactive
    @SeriyBg SeriyBg temporarily deployed to report July 11, 2023 11:43 — with GitHub Actions Inactive
    @github-actions
    Copy link

    Test Results

    Total Tests 🔴 Failures 🟠 Errors ⚪ Skipped
    OS 93 0 0 45
    EE 93 1 0 14
    Failed Tests
    OS
    EE
  • Hazelcast Hazelcast CR TLS configuration when TLS with Mutual Authentication property is configured should form a cluster and be able to connect
    		•

    @SeriyBg SeriyBg temporarily deployed to report July 11, 2023 12:06 — with GitHub Actions Inactive
    @SeriyBg SeriyBg temporarily deployed to report July 11, 2023 13:44 — with GitHub Actions Inactive
    @SeriyBg SeriyBg merged commit 28b9921 into main Jul 12, 2023
    18 checks passed
    @SeriyBg SeriyBg deleted the CN-902-wan-validation branch July 12, 2023 10:13
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @semihbkgr semihbkgr semihbkgr approved these changes

    @dzeromski-hazelcast dzeromski-hazelcast dzeromski-hazelcast approved these changes

    Assignees
    No one assigned
    Labels
    enhancement New feature or request
    Projects
    None yet
    Milestone
    5.9
    Development

    Successfully merging this pull request may close these issues.

    None yet
    3 participants
    @SeriyBg @semihbkgr @dzeromski-hazelcast