Skip to content

Conversation

@pravisankar
Copy link

@pravisankar pravisankar commented Nov 1, 2018

  • Currently not supported by openshift dns operator
    This functionality is needed to ensure backward compatibility.

  • Extended tests should honor SKIP env when FOCUS env is set

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 1, 2018
@pravisankar
Copy link
Author

@openshift/sig-network-edge PTAL

@ironcladlou
Copy link
Contributor

@smarterclayton is it correct that we need this test to continue working as-is against 3.x Ansible-installed clusters? If so, I can think of a couple ways to make sure the suite passes against both:

  1. Add stuff to the test to skip when we detect 4.0 router
  2. Actively skip the unsupported test in the operator CI config

Other ways/thoughts?

@smarterclayton
Copy link
Contributor

smarterclayton commented Nov 1, 2018 via email

@ironcladlou
Copy link
Contributor

Did we get the necessary approval to break backwards compatibility for this
feature by verifying customers weren’t affected, measuring the cost to
support it, what workarounds exist? Or are you asking about temporary
disablement?

Just temporary.

f := e2e.NewDefaultFramework("dns")

It("should answer endpoint and wildcard queries for the cluster [Conformance]", func() {
Skip("TODO: re-enable when wildcard dns queries are supported by openshift dns operator")
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of changing it here, add this to test/extended/util/test.go and add it to the section on broken tests with a link to the enablement.

No one should be adding Skip rules to their tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

edit: unless they are conditional skips (this isn't conditional)

@pravisankar pravisankar force-pushed the skip-dns-wildcard-test branch from 17eb31e to d957353 Compare November 7, 2018 15:03
@pravisankar pravisankar changed the title Skip wildcard dns extended test Disable wildcard dns extended test Nov 7, 2018
@pravisankar
Copy link
Author

@ironcladlou @smarterclayton updated, PTAL

local skip="\[Serial\]"
if [[ -n "${SKIP:-}" ]]; then
skip+="|${SKIP}"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to imply there was a bug of some sort?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, without this fix, ginko command will be:

ginkgo -v -noColor -focus=DNS -skip=\[Disabled:.+\]|\[Disruptive\]|\[Skipped\]|\[Slow\]|\[Flaky\]|\[local\]|\[Local\] -p -nodes=5  os::util::find::built_binary extended.test  -- -ginkgo.skip \[Serial\] -test.timeout 6h

Later ginkgo.skip option overrides the former -skip option and we do see execution of wildcard dns queries that has [Disabled:Broken] tag.

@ironcladlou
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 7, 2018
@openshift-bot
Copy link
Contributor

/retest

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

# first run anything that isn't explicitly declared [Serial], and matches the $FOCUS, in a parallel mode.
os::log::info "Running parallel tests N=${PARALLEL_NODES:-<default>} with focus ${FOCUS}"
TEST_REPORT_FILE_NAME=focus_parallel TEST_PARALLEL="${PARALLEL_NODES:-5}" os::test::extended::run -- -ginkgo.skip "\[Serial\]" -test.timeout 6h ${TEST_EXTENDED_ARGS-} || exitstatus=$?
TEST_REPORT_FILE_NAME=focus_parallel TEST_PARALLEL="${PARALLEL_NODES:-5}" os::test::extended::run -- -ginkgo.skip ${skip} -test.timeout 6h ${TEST_EXTENDED_ARGS-} || exitstatus=$?
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to wrap ${skip} in double-quotes in case the user specifies a value with spaces.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@openshift-bot
Copy link
Contributor

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

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

Ravi Sankar Penta added 2 commits November 7, 2018 19:51
- Currently not supported by openshift dns operator
This functionality is needed to ensure backward compatibility.
@pravisankar pravisankar force-pushed the skip-dns-wildcard-test branch from d957353 to 86c095d Compare November 8, 2018 03:51
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2018
@pravisankar
Copy link
Author

/retest

2 similar comments
@pravisankar
Copy link
Author

/retest

@ironcladlou
Copy link
Contributor

/retest

@ironcladlou
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, pravisankar

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

@pravisankar
Copy link
Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 027d88e into openshift:master Nov 8, 2018
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. lgtm Indicates that a PR is ready to be merged. sig/network-edge size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants