Point tests to the latest nightly eventing-sources#719
Conversation
knative-prow-robot
added
the
size/XS
|
/lgtm I wonder if we should be using the latest release, rather than the latest nightly, but either seems better than a random release a month ago. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bbrowning, evankanderson The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
|
The pin was originally done because eventing-sources was causing the eventing e2e tests to fail every time, and that was blocking all eventing PRs (#683). I investigated for a while but eventually got pulled away without a solution. I also tried to copy the eventing e2e tests to eventing-sources (knative/eventing-contrib#121) but never got those working either. This temporary unblocker should not have stuck around so long and I take responsibility for never coming back to fix it (it should have been added to the 0.3.0 milestone as a blocker). As @bbrowning points out, it probably means that CI has never tested 0.3 eventing and 0.3 eventing-sources together. The root cause is probably knative/eventing-contrib#137. Unfortunately the eventing e2e tests are really hard to debug so I never found out exactly what the issue was. It's likely that the quickest way forward is attempting to back out those changes - they don't add any functionality, just refactor the existing stuff. There will be conflicts because some later sources rely on the new signature. |
I'm attempting this now. |
|
@grantr Thanks for jumping in. We all missed reverting this so don't fret about that. It's not obvious why this integration test failed, but given the reports we've see with k8sevents in 0.3 from a couple of people already on knative/docs#733, we may have a similar issue. I'm pushing on with the E2E test work in eventing-sources as knative/eventing-contrib#178 in an effort to get something going there to help prevent this from happening again. |
|
knative/eventing-contrib#179 reverts most eventing-sources (the k8s source in particular) to use the old GetSinkURI implementation. If the eventing e2e tests pass with this change, it seems likely that knative/eventing-contrib#137 is causing the failures. |
|
I created #720 which pins eventing-sources to the state of knative/eventing-contrib#179. It passes for me when I run the tests from my command line (with a GKE cluster). I'm pretty certain now that the failure stems from knative/eventing-contrib#137 (though I don't know why). |
We had an odd mix of defaultNamespaceName and pkgTest.Flags.Namespace in the e2e tests. The defaultNamespaceName is meant to supply the default value for pkgTest.Flags.Namespace, but we should have been referencing pkgTest.Flags.Namespace everywhere as it may not be the same as the default value when running in CI.
|
New changes are detected. LGTM label has been removed. |
knative-prow-robot
added
size/S
|
So, knative/eventing-contrib#178 got the Kubernetes event source E2E test passing in the eventing-sources repo. Which is odd, since that same test failed here when pointing at a recent eventing-sources release. Doing some digging, there were some slight differences in how the eventing test mixes direct usage of At any rate, I added a commit here to fix the namespace references and we'll see if that gets this test passing or if it's something else that allows eventing-sources to pass this test but eventing to not. |
|
That had no difference. So, I'm a bit stumped as to how this test passes in eventing-sources but not in eventing. |
|
@bbrowning: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
This can now be closed because we no longer have a dependency on the eventing-sources? @bbrowning that's right, yes? |
|
Closing as superseded by #726. @bbrowning reopen if desired. |
|
Not depending on eventing-sources is even better 👍 |
6 participants
Fixes #718