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

baremetal: fail if apiVIP and ingressVIP are set to the same value #5186

Merged
merged 1 commit into from
Oct 26, 2021
Merged

baremetal: fail if apiVIP and ingressVIP are set to the same value #5186

merged 1 commit into from
Oct 26, 2021

Conversation

karmab
Copy link
Contributor

@karmab karmab commented Sep 2, 2021

baremetal: fail if apiVIP and ingressVIP are set to the same value

@hardys
Copy link
Contributor

hardys commented Sep 2, 2021

Looks good, thanks! Would you mind adding a test please? https://github.com/openshift/installer/blob/master/pkg/types/baremetal/validation/platform_test.go#L45

@hardys
Copy link
Contributor

hardys commented Sep 2, 2021

/label platform/baremetal

@openshift-ci openshift-ci bot added the platform/baremetal IPI bare metal hosts platform label Sep 2, 2021
@karmab karmab force-pushed the baremetal_dedicated_vip branch 2 times, most recently from 8357e3f to 4f431ff Compare September 2, 2021 20:21
@karmab
Copy link
Contributor Author

karmab commented Sep 2, 2021

Looks good, thanks! Would you mind adding a test please? https://github.com/openshift/installer/blob/master/pkg/types/baremetal/validation/platform_test.go#L45

done!

@sadasu
Copy link
Contributor

sadasu commented Sep 7, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2021
@hardys
Copy link
Contributor

hardys commented Sep 8, 2021

Thanks @karmab - lgtm but there seems to be a mismatch in the expected error in the new test:

             	Error Trace:	platform_test.go:172
            	Error:      	Expect "baremetal.apiVIP: Invalid value: "192.168.111.100": apiVIP and ingressVIP must not be set to the same value" to match "Incorrect value: "192.168.111.100": APIVIP is expected to be different than IngressVIP"
            	Test:       	TestValidatePlatform/identical_apivip_ingressvip

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 1, 2021
@karmab
Copy link
Contributor Author

karmab commented Oct 1, 2021

Thanks @karmab - lgtm but there seems to be a mismatch in the expected error in the new test:

             	Error Trace:	platform_test.go:172
            	Error:      	Expect "baremetal.apiVIP: Invalid value: "192.168.111.100": apiVIP and ingressVIP must not be set to the same value" to match "Incorrect value: "192.168.111.100": APIVIP is expected to be different than IngressVIP"
            	Test:       	TestValidatePlatform/identical_apivip_ingressvip

my bad, i misinterpreted the test logic.
Fixed!

@hardys
Copy link
Contributor

hardys commented Oct 21, 2021

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2021
@hardys
Copy link
Contributor

hardys commented Oct 21, 2021

/test e2e-metal-ipi-ovn-dualstack

ardaguclu
ardaguclu reviewed Oct 26, 2021
View reviewed changes
Copy link
Member

@ardaguclu ardaguclu left a comment

Choose a reason for hiding this comment

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

Thanks @karmab

pkg/types/baremetal/validation/platform.go
@@ -355,6 +362,10 @@ func ValidatePlatform(p *baremetal.Platform, n *types.Networking, fldPath *field
allErrs = append(allErrs, field.Required(fldPath.Child("Hosts"), err.Error()))
}

if err := validateAPIVIPIsDifferentFromINGRESSVIP(p.APIVIP, p.IngressVIP); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it better to rename this validateAPIVIPIsDifferentFromIngressVIP?

kirankt
kirankt approved these changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@kirankt kirankt left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 26, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hardys, kirankt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 26, 2021 •
edited
Loading

@karmab: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-openstack-provider-network 4f431ff link /test e2e-openstack-provider-network
ci/prow/e2e-aws-workers-rhel8 5744258 link false /test e2e-aws-workers-rhel8
ci/prow/e2e-metal-single-node-live-iso 5744258 link false /test e2e-metal-single-node-live-iso

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

8 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 1b4b561 into openshift:master Oct 26, 2021
@karmab karmab deleted the baremetal_dedicated_vip branch October 29, 2021 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ardaguclu ardaguclu ardaguclu left review comments

@kirankt kirankt kirankt approved these changes

@celebdor celebdor Awaiting requested review from celebdor

@russellb russellb Awaiting requested review from russellb

Assignees

@sadasu sadasu

@kirankt kirankt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. platform/baremetal IPI bare metal hosts platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@karmab @hardys @sadasu @openshift-bot @ardaguclu @kirankt @openshift-merge-robot