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.

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

cp 3897 #3902

Merged
merged 5 commits into from
Aug 21, 2020
Merged

cp 3897 #3902

merged 5 commits into from
Aug 21, 2020

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented Aug 20, 2020

Addresses #3791

When send fails or response is nil, set the response code to 0.

Proposed Changes

Release Note

- 🐛 Fix bug
In cases where Filter sends a message and it fails or response is nil, it will panic because it uses it.

Docs

@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 20, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 20, 2020
@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2020
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2020
@vaikas
Copy link
Contributor Author

vaikas commented Aug 20, 2020

wat?

f0b72b24-e32f-11ea-b588-76fe22e39175
./test/../vendor/knative.dev/test-infra/scripts/presubmit-tests.sh: line 376: ./test/e2e-conformance-tests.sh: No such file or directory
Step failed: ./test/e2e-conformance-tests.sh
+ EXIT_VALUE=1

/test pull-knative-eventing-conformance-tests

@chizhg
Copy link
Member

chizhg commented Aug 21, 2020

/override pull-knative-eventing-conformance-tests

@knative-prow-robot
Copy link
Contributor

@chizhg: chizhg unauthorized: /override is restricted to Repo administrators.

In response to this:

/override pull-knative-eventing-conformance-tests

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.

@vaikas
Copy link
Contributor Author

vaikas commented Aug 21, 2020

/override pull-knative-eventing-conformance-tests

@knative-prow-robot
Copy link
Contributor

@vaikas: vaikas unauthorized: /override is restricted to Repo administrators.

In response to this:

/override pull-knative-eventing-conformance-tests

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.

@vaikas
Copy link
Contributor Author

vaikas commented Aug 21, 2020

As discussed here, this is due to change in how the jobs get run. It applies to all the branches and this version does not have the job in there.
https://knative.slack.com/archives/C9JP909F0/p1597962909008400

@knative-prow-robot knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release and removed lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 21, 2020
@vaikas
Copy link
Contributor Author

vaikas commented Aug 21, 2020

Broker didn't become ready, but there are no knative-eventing logs.
/test pull-knative-eventing-conformance-tests

initialize $@ --skip-istio-addon

echo "Running tests with Multi Tenant Channel Based Broker"
go_test_e2e -timeout=30m -parallel=12 ./test/conformance -brokerclass=MTChannelBasedBroker -channels=messaging.knative.dev/v1beta1:Channel,messaging.knative.dev/v1beta1:InMemoryChannel,messaging.knative.dev/v1:Channel,messaging.knative.dev/v1:InMemoryChannel -sources=sources.knative.dev/v1beta1:ApiServerSource,sources.knative.dev/v1alpha2:ContainerSource,sources.knative.dev/v1alpha2:PingSource || fail_test
Copy link
Member

Choose a reason for hiding this comment

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

why the difference between source gvk's?
-sources=sources.knative.dev/v1beta1:ApiServerSource,sources.knative.dev/v1alpha2:ContainerSource,sources.knative.dev/v1alpha2:PingSource
vs
-sources=sources.knative.dev/v1alpha2:ApiServerSource,sources.knative.dev/v1alpha2:ContainerSource,sources.knative.dev/v1alpha2:PingSource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what was in the head. Great catch, changed :)

@knative-test-reporter-robot

The following jobs failed:

Test name Triggers Retries
pull-knative-eventing-conformance-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-eventing-conformance-tests:

test/conformance.TestBrokerV1Beta1ControlPlane
test/conformance.TestBrokerTracing
test/conformance.TestBrokerV1Beta1ControlPlane/Channel-messaging.knative.dev/v1beta1
test/conformance.TestBrokerTracing/Channel-messaging.knative.dev/v1beta1
test/conformance.TestBrokerV1Beta1ControlPlane/InMemoryChannel-messaging.knative.dev/v1beta1
test/conformance.TestBrokerTracing/InMemoryChannel-messaging.knative.dev/v1beta1
test/conformance.TestBrokerV1Beta1ControlPlane/Channel-messaging.knative.dev/v1
test/conformance.TestBrokerTracing/Channel-messaging.knative.dev/v1

and 6 more.

@lberk
Copy link
Member

lberk commented Aug 21, 2020

All pods are up:
eventing-controller-6fd69c59db-bs22t   1/1   Running   0     2m21s
eventing-webhook-76f4859c5b-wr745      1/1   Running   1     2m20s
imc-controller-747646d968-n5246        1/1   Running   0     5s
imc-dispatcher-59d4d54bf6-s9bww        1/1   Running   0     5s

is the mt-channel broker yaml being applied? is the controller being brought up? seems weird that all the broker tests (even the tracing ones) are failing with the broker not being created

@lberk
Copy link
Member

lberk commented Aug 21, 2020

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 21, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, lberk, vaikas

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

@knative-prow-robot knative-prow-robot merged commit 62aa591 into knative:release-0.16 Aug 21, 2020
@vaikas vaikas deleted the cp-3897-to-16 branch August 21, 2020 19:28
@lberk lberk mentioned this pull request Sep 1, 2020
matzew pushed a commit to matzew/eventing that referenced this pull request Sep 30, 2020
* cp 3897

* backport 3719

* x bit

* apiserversource v1beta1->v1alpha2

* install broker / sugar
matzew pushed a commit to matzew/eventing that referenced this pull request Oct 1, 2020
* cp 3897

* backport 3719

* x bit

* apiserversource v1beta1->v1alpha2

* install broker / sugar
openshift-merge-robot pushed a commit to openshift/knative-eventing that referenced this pull request Oct 7, 2020
* adding istion annotation (knative#3534) (knative#3557)

* adding istion annotation (knative#3534)

* backporting knative#3543

* Update test/lib/duck/resource_inspectors.go

Co-authored-by: Matt Moore <[email protected]>

* Use the standard resource creation libs for upgrade tests. (knative#3539)

* try to dump some state

* use client wait or fail

* create broker directly

* removed the unused import

Co-authored-by: Matt Moore <[email protected]>

* cp 3897 (knative#3902)

* cp 3897

* backport 3719

* x bit

* apiserversource v1beta1->v1alpha2

* install broker / sugar

* backport 3945 to .16 (knative#3948)

Co-authored-by: VijayaSenaReddy Ayyappaneni <[email protected]>
Co-authored-by: Matt Moore <[email protected]>
Co-authored-by: Ville Aikas <[email protected]>
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. area/test-and-release Test infrastructure, tests or release cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants