Skip to content

Bug 1971590: Enable TestEndpointAdmission test only for OpenShift SDN#26223

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
ardaguclu:fix-testadmission-ipv6
Jul 27, 2021
Merged

Bug 1971590: Enable TestEndpointAdmission test only for OpenShift SDN#26223
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
ardaguclu:fix-testadmission-ipv6

Conversation

@ardaguclu
Copy link
Member

@ardaguclu ardaguclu commented Jun 14, 2021

This PR applies only enabling TestEndpointAdmission test for OpenShift SDN

@openshift-ci openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium 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 14, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 14, 2021

@ardaguclu: This pull request references Bugzilla bug 1971590, which is invalid:

  • expected the bug to target the "4.9.0" release, but it targets "4.8.0" 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.

Details

In response to this:

Bug 1971590: Add IPv6 endpoint for TestEndpointAdmission test

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.

@openshift-ci openshift-ci bot requested review from dcbw and squeed June 14, 2021 11:32
@ardaguclu
Copy link
Member Author

/retest

@ardaguclu
Copy link
Member Author

/bugzilla refresh

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 14, 2021

@ardaguclu: This pull request references Bugzilla bug 1971590, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.9.0) matches configured target release for branch (4.9.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
Details

In response to this:

/bugzilla refresh

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.

@openshift-ci openshift-ci bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jun 14, 2021
@ardaguclu
Copy link
Member Author

/retest

"cluster": "10.128.0.2",
"service": "172.30.0.2",
"external": "1.2.3.4",
var exampleServiceAddresses = map[string]string{
Copy link
Contributor

Choose a reason for hiding this comment

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

@ardaguclu I would move this into the TestEndpointAdmission() function since it's only used there and it's confusing to have it as a global when testOne() also creates a map of the same name, but in local scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

In fact, why even bother with the map? Why not just assign the addresses directly since exampleServiceAddresses is only used in one place?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for review @dcbw, I changed according to your suggestions.

@ardaguclu ardaguclu force-pushed the fix-testadmission-ipv6 branch from 42eae84 to ea77745 Compare June 15, 2021 07:20
@ardaguclu
Copy link
Member Author

/retest

@ardaguclu
Copy link
Member Author

/test e2e-aws-csi

Copy link
Contributor

@danwinship danwinship left a comment

Choose a reason for hiding this comment

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

There was discussion about this test somewhere else but I don't remember where.

This is only relevant for openshift-sdn in Multitenant mode, and thus there's no need to test it under IPv6. The test should just be wrapped in InOpenShiftSDNContext() so it gets skipped for other plugins.

@ardaguclu ardaguclu force-pushed the fix-testadmission-ipv6 branch 2 times, most recently from 5e62e23 to e993ed3 Compare June 25, 2021 12:41
@ardaguclu ardaguclu changed the title Bug 1971590: Add IPv6 endpoint for TestEndpointAdmission test Bug 1971590: Enable TestEndpointAdmission test only for OpenShift SDN Jun 25, 2021
@ardaguclu
Copy link
Member Author

Thanks @danwinship, I did changes and only enable this test for OpenShift sdn is enabled cases.

@ardaguclu
Copy link
Member Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jun 29, 2021

@ardaguclu: This pull request references Bugzilla bug 1971590, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.9.0) matches configured target release for branch (4.9.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
Details

In response to this:

Bug 1971590: Enable TestEndpointAdmission test only for OpenShift SDN

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.

@ardaguclu
Copy link
Member Author

/retest

1 similar comment
@ardaguclu
Copy link
Member Author

/retest

g.It("TestEndpointAdmission", func() {
TestEndpointAdmission(g.GinkgoT(), oc)
InOpenShiftSDNContext(func() {
g.It("TestEndpointAdmission", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe clarify that while we're here

g.It("blocks manual creation of Endpoints pointing to the cluster or service network", func() {

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Thanks.

@ardaguclu ardaguclu force-pushed the fix-testadmission-ipv6 branch from e993ed3 to 7385524 Compare July 14, 2021 05:30
@ardaguclu
Copy link
Member Author

/retest

@danwinship
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2021
@ardaguclu
Copy link
Member Author

/test e2e-gcp-upgrade

@danwinship
Copy link
Contributor

/assign @knobunc

@knobunc
Copy link
Contributor

knobunc commented Jul 26, 2021

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, danwinship, knobunc

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

The pull request process is described here

Details 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 Jul 26, 2021
@ardaguclu
Copy link
Member Author

/retest

@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.

@ardaguclu
Copy link
Member Author

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 26, 2021

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

Test name Commit Details Rerun command
ci/prow/e2e-gcp-disruptive 42eae84 link /test e2e-gcp-disruptive
ci/prow/e2e-aws-disruptive 42eae84 link /test e2e-aws-disruptive

Full PR test history. Your PR dashboard.

Details

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.

@openshift-merge-robot openshift-merge-robot merged commit 9efa552 into openshift:master Jul 27, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2021

@ardaguclu: All pull requests linked via external trackers have merged:

Bugzilla bug 1971590 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1971590: Enable TestEndpointAdmission test only for OpenShift SDN

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.

@ardaguclu
Copy link
Member Author

/cherrypick release-4.8

@openshift-cherrypick-robot

@ardaguclu: new pull request created: #26351

Details

In response to this:

/cherrypick release-4.8

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants