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

OpenStack: Add Unit Tests for validation of platform.openstack.machineSubnet #4937

Merged
merged 2 commits into from Jun 21, 2021
Merged

OpenStack: Add Unit Tests for validation of platform.openstack.machineSubnet #4937

merged 2 commits into from Jun 21, 2021

Conversation

gyohuangxin
Copy link
Contributor

  1. Add Unit Tests for validation of platform.openstack.machineSubnet.
  2. Delete the duplicate logic in platform.openstack.machineSubnet

    installer/pkg/asset/installconfig/openstack/validation/platform.go

    Lines 52 to 54 in aba6041

    if len(p.ExternalDNS) > 0 && p.MachinesSubnet != "" {
    allErrs = append(allErrs, field.InternalError(fldPath.Child("machinesSubnet"), fmt.Errorf("externalDNS can't be set when using a custom machinesSubnet")))
    }

@openshift-ci openshift-ci bot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels May 16, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 16, 2021

@gyohuangxin: This pull request references Bugzilla bug 1957809, which is invalid:

  • expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is ON_QA instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1957809: Add Unit Tests for validation of platform.openstack.machineSubnet

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.

@gyohuangxin
Copy link
Contributor Author

/retest

1 similar comment
@gyohuangxin
Copy link
Contributor Author

/retest

@gyohuangxin
Copy link
Contributor Author

gyohuangxin commented May 19, 2021 •
edited

@mandre Could you review this? Thanks.

@gyohuangxin
Copy link
Contributor Author

/retest

5 similar comments
@gyohuangxin
Copy link
Contributor Author

/retest

@gyohuangxin
Copy link
Contributor Author

/retest

@gyohuangxin
Copy link
Contributor Author

/retest

@gyohuangxin
Copy link
Contributor Author

/retest

@gyohuangxin
Copy link
Contributor Author

/retest

mandre
mandre suggested changes Jun 17, 2021
View reviewed changes
pkg/asset/installconfig/openstack/validation/platform_test.go Outdated
networking: func() *types.Networking {
n := validNetworking()
cidr := ipnet.IPNet{
net.IPNet{
Copy link
Member

Choose a reason for hiding this comment

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

ditto

pkg/asset/installconfig/openstack/validation/platform.go
@@ -49,9 +49,6 @@ func validateMachinesSubnet(p *openstack.Platform, n *types.Networking, ci *Clou
}
}

if len(p.ExternalDNS) > 0 && p.MachinesSubnet != "" {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this is unrelated to the topic of your commit which is to add unit tests.
Ideally you would make this change in a separate commit (in the same PR) and explain why it's OK to remove the validation and perhaps link to the other validation that already covers that case in your commit message.

https://github.com/openshift/installer/blob/6d5a001/pkg/asset/installconfig/openstack/validation/platform.go#L41-L43

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I've put it into a new commit and explain the reason in commit message.

pkg/asset/installconfig/openstack/validation/platform_test.go Outdated
Comment on lines 392 to 406
cidr := ipnet.IPNet{
net.IPNet{
IP: net.IPv4(172, 0, 0, 1),
Mask: net.IPv4Mask(255, 255, 255, 0),
},
}
machineNetworkEntry := &types.MachineNetworkEntry{
CIDR: cidr,
}
Copy link
Member

Choose a reason for hiding this comment

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

This causes govet to fail:

pkg/asset/installconfig/openstack/validation/platform_test.go:405:13: github.com/openshift/installer/pkg/ipnet.IPNet composite literal uses unkeyed fields
pkg/asset/installconfig/openstack/validation/platform_test.go:436:13: github.com/openshift/installer/pkg/ipnet.IPNet composite literal uses unkeyed fields

Try this:

Suggested change
cidr := ipnet.IPNet{
net.IPNet{
IP: net.IPv4(172, 0, 0, 1),
Mask: net.IPv4Mask(255, 255, 255, 0),
},
}
machineNetworkEntry := &types.MachineNetworkEntry{
CIDR: cidr,
}
machineNetworkEntry := &types.MachineNetworkEntry{
CIDR: *ipnet.MustParseCIDR("172.0.0.1/24"),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks so much, it confused me for a long time. Let's wait and see:)

@gyohuangxin
Copy link
Contributor Author

/retest

2 similar comments
@gyohuangxin
Copy link
Contributor Author

/retest

@gyohuangxin
Copy link
Contributor Author

/retest

@mandre
Copy link
Member

mandre commented Jun 18, 2021

/retitle OpenStack: Add Unit Tests for validation of platform.openstack.machineSubnet

@openshift-ci openshift-ci bot changed the title Bug 1957809: Add Unit Tests for validation of platform.openstack.machineSubnet OpenStack: Add Unit Tests for validation of platform.openstack.machineSubnet Jun 18, 2021
@openshift-ci openshift-ci bot removed bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jun 18, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 18, 2021

@gyohuangxin: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

OpenStack: Add Unit Tests for validation of platform.openstack.machineSubnet

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.

@mandre
Copy link
Member

mandre commented Jun 18, 2021

We're currently having issues with our e2e-openstack, let's use e2e-openstack-ipi instead
/approve
/test e2e-openstack-ipi

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mandre

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2021
@gyohuangxin
Copy link
Contributor Author

/retest

@gyohuangxin
Copy link
Contributor Author

@mandre Hi, all tests except e2e-openstack has passed. Can you merge this? Thanks!

mandre
mandre reviewed Jun 21, 2021
View reviewed changes
Copy link
Member

@mandre mandre left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks for the patch 🎉

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

/retest

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

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 21, 2021

@gyohuangxin: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-openstack b750bfd link /test e2e-openstack

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-merge-robot openshift-merge-robot merged commit b06ea6f into openshift:master Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mandre mandre mandre requested changes

@pierreprinetti pierreprinetti Awaiting requested review from pierreprinetti

Assignees

@mandre mandre

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gyohuangxin @mandre @openshift-bot @openshift-merge-robot