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

do not hardcode the URI, use ref instead #4278

Merged
merged 1 commit into from
Oct 12, 2020

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented Oct 12, 2020

Proposed Changes

Release Note

- 🧽 Update or clean up current behavior
Point to Broker ref instead of using a hard coded path. Also makes things little easier to reuse against other brokers.

Docs

@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 12, 2020
@knative-prow-robot knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 12, 2020
@knative-prow-robot knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Oct 12, 2020
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2020
@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #4278 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4278   +/-   ##
=======================================
  Coverage   80.25%   80.25%           
=======================================
  Files         287      287           
  Lines        7887     7887           
=======================================
  Hits         6330     6330           
  Misses       1174     1174           
  Partials      383      383           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c1d8d7...f9ae776. Read the comment docs.

Copy link
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

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

I like using rhe ref!

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, 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

@vaikas
Copy link
Contributor Author

vaikas commented Oct 12, 2020

=== CONT  TestBrokerChannelFlowTriggerV1BrokerV1Beta1/InMemoryChannel-messaging.knative.dev/v1
    operation.go:121: Waiting for 1 endpoints in service e2e-brokerchannel-logger-pod1
    creation.go:409: Creating v1 trigger e2e-brokerchannel-trigger2
    creation.go:80: Creating channel &TypeMeta{Kind:InMemoryChannel,APIVersion:messaging.knative.dev/v1,}-e2e-brokerchannel-channel
    broker_channel_flow_helper.go:160: Failed to get the url for the channel "e2e-brokerchannel-channel": addressable does not have an Address: &{TypeMeta:{Kind:InMemoryChannel APIVersion:messaging.knative.dev/v1} ObjectMeta:{Name:e2e-brokerchannel-channel GenerateName: Namespace:test-broker-channel-flow-trigger-v1-broker-v1-beta1-in-memvfkvz SelfLink:/apis/messaging.knative.dev/v1/namespaces/test-broker-channel-flow-trigger-v1-broker-v1-beta1-in-memvfkvz/inmemorychannels/e2e-brokerchannel-channel UID:49bf5386-b075-4868-9f8e-109eb65d8296 ResourceVersion:33564 Generation:1 CreationTimestamp:2020-10-12 08:10:08 +0000 UTC DeletionTimestamp:<nil> DeletionGracePeriodSeconds:<nil> Labels:map[] Annotations:map[messaging.knative.dev/creator:[email protected] messaging.knative.dev/lastModifier:[email protected] messaging.knative.dev/subscribable:v1] OwnerReferences:[] Finalizers:[] ClusterName: ManagedFields:[]} Status:{Address:<nil>}}
        knative.dev/eventing/test/lib.(*Client).GetAddressableURI
        	/home/prow/go/src/knative.dev/eventing/test/lib/operation.go:57
        knative.dev/eventing/test/e2e/helpers.BrokerChannelFlowWithTransformation.func1
        	/home/prow/go/src/knative.dev/eventing/test/e2e/helpers/broker_channel_flow_helper.go:158
        knative.dev/eventing/test/lib.(*ComponentsTestRunner).RunTests.func1
        	/home/prow/go/src/knative.dev/eventing/test/lib/test_runner.go:75
        testing.tRunner
        	/root/.gvm/gos/go1.14.6/src/testing/testing.go:1039
        runtime.goexit
        	/root/.gvm/gos/go1.14.6/src/runtime/asm_amd64.s:1373

And then.

    tracker.go:129: Cleaning resource: "e2e-brokerchannel-channel"
        {
          "apiVersion": "messaging.knative.dev/v1",
          "kind": "InMemoryChannel",
          "metadata": {
            "annotations": {
              "messaging.knative.dev/creator": "[email protected]",
              "messaging.knative.dev/lastModifier": "[email protected]",
              "messaging.knative.dev/subscribable": "v1"
            },
            "creationTimestamp": "2020-10-12T08:10:08Z",
            "generation": 1,
            "name": "e2e-brokerchannel-channel",
            "namespace": "test-broker-channel-flow-trigger-v1-broker-v1-beta1-in-memvfkvz",
            "resourceVersion": "33564",
            "selfLink": "/apis/messaging.knative.dev/v1/namespaces/test-broker-channel-flow-trigger-v1-broker-v1-beta1-in-memvfkvz/inmemorychannels/e2e-brokerchannel-channel",
            "uid": "49bf5386-b075-4868-9f8e-109eb65d8296"
          },
          "spec": {},
          "status": {
            "conditions": [
              {
                "lastTransitionTime": "2020-10-12T08:10:08Z",
                "status": "Unknown",
                "type": "Addressable"
              },
              {
                "lastTransitionTime": "2020-10-12T08:10:08Z",
                "status": "Unknown",
                "type": "ChannelServiceReady"
              },
              {
                "lastTransitionTime": "2020-10-12T08:10:08Z",
                "status": "Unknown",
                "type": "DispatcherReady"
              },
              {
                "lastTransitionTime": "2020-10-12T08:10:08Z",
                "status": "Unknown",
                "type": "EndpointsReady"
              },
              {
                "lastTransitionTime": "2020-10-12T08:10:08Z",
                "message": "unsuccessfully observed a new generation",
                "reason": "NewObservedGenFailure",
                "status": "Unknown",
                "type": "Ready"
              },
              {
                "lastTransitionTime": "2020-10-12T08:10:08Z",
                "status": "Unknown",
                "type": "ServiceReady"
              }
            ],
            "observedGeneration": 1
          }
        }

@vaikas
Copy link
Contributor Author

vaikas commented Oct 12, 2020

/test pull-knative-eventing-integration-tests

@knative-prow-robot knative-prow-robot merged commit 1ef177f into knative:master Oct 12, 2020
@vaikas vaikas deleted the use-ref-instead-of-uri branch October 13, 2020 10:46
knative-prow bot pushed a commit that referenced this pull request Jan 20, 2023
* Print "Fetching trace ..." at DEBUG level because we already print
another message "Exporting trace into /file/path/step-XXXX.json" at INFO
level which is sufficient.
* Print "Duplicate events: event #XXXX received X times, but should be
received only once" at INFO level to get rid of the stacktrace that is
printed at WARN level. This stack trace is really noisy and not useful
at all. And with tens to hundreds of duplicate events it makes the logs
really long.

This will reduce the log in the following way for each duplicate event:

Before:
```
2023-01-19T09:54:40.013-0500	WARN	prober/verify.go:99	Duplicate events: event #4278 received 2 times, but should be received only once
knative.dev/eventing/test/upgrade/prober.(*prober).Verify
	/tmp/gopath-ZSUHshgs6a/src/github.com/openshift-knative/serverless-operator/vendor/knative.dev/eventing/test/upgrade/prober/verify.go:99
knative.dev/eventing/test/upgrade/prober.(*probeRunner).Verify
	/tmp/gopath-ZSUHshgs6a/src/github.com/openshift-knative/serverless-operator/vendor/knative.dev/eventing/test/upgrade/prober/prober.go:74
knative.dev/eventing/test/upgrade/prober.NewContinualVerification.func2
	/tmp/gopath-ZSUHshgs6a/src/github.com/openshift-knative/serverless-operator/vendor/knative.dev/eventing/test/upgrade/prober/continual.go:56
knative.dev/pkg/test/upgrade.NewBackgroundVerification.func1.1
	/tmp/gopath-ZSUHshgs6a/src/github.com/openshift-knative/serverless-operator/vendor/knative.dev/pkg/test/upgrade/functions.go:65
knative.dev/pkg/test/upgrade.handleStopEvent
	/tmp/gopath-ZSUHshgs6a/src/github.com/openshift-knative/serverless-operator/vendor/knative.dev/pkg/test/upgrade/functions.go:126
knative.dev/pkg/test/upgrade.WaitForStopEvent
	/tmp/gopath-ZSUHshgs6a/src/github.com/openshift-knative/serverless-operator/vendor/knative.dev/pkg/test/upgrade/functions.go:94
knative.dev/pkg/test/upgrade.NewBackgroundVerification.func1
	/tmp/gopath-ZSUHshgs6a/src/github.com/openshift-knative/serverless-operator/vendor/knative.dev/pkg/test/upgrade/functions.go:62
knative.dev/pkg/test/upgrade.(*suiteExecution).startContinualTests.func1.2
	/tmp/gopath-ZSUHshgs6a/src/github.com/openshift-knative/serverless-operator/vendor/knative.dev/pkg/test/upgrade/steps.go:81
2023-01-19T09:54:40.013-0500	INFO	prober/verify.go:135	Fetching trace for Step event #4278
2023-01-19T09:54:42.593-0500	INFO	prober/verify.go:168	Exporting trace into /home/jenkins/workspace/functional_tests/stream1_27/rolling-upgrade-1.27/artifacts/build-18/traces/events/step-4278.json

```

After:
```
2023-01-19T09:54:40.013-0500	INFO	prober/verify.go:99	WARNING: Duplicate: event #4278 received 2 times, but should be received only once
2023-01-19T09:54:42.593-0500	INFO	prober/verify.go:168	Exporting trace into /home/jenkins/workspace/functional_tests/stream1_27/rolling-upgrade-1.27/artifacts/build-18/traces/events/step-4278.json

```

Fixes #

<!-- Please include the 'why' behind your changes if no issue exists -->

## Proposed Changes

<!-- Please categorize your changes:
- :gift: Add new feature
- :bug: Fix bug
- :broom: Update or clean up current behavior
- :wastebasket: Remove feature or internal logic
-->

-
-
-

### Pre-review Checklist

<!-- If these boxes are not checked, you will be asked to complete these
requirements or explain why they do not apply to your PR. -->

- [ ] **At least 80% unit test coverage**
- [ ] **E2E tests** for any new behavior
- [ ] **Docs PR** for any user-facing impact
- [ ] **Spec PR** for any new API feature
- [ ] **Conformance test** for any change to the spec

**Release Note**

<!--
:page_facing_up: If this change has user-visible impact, write a release
note in the block
below. Include the string "action required" if additional action is
required of
users switching to the new release, for example in case of a breaking
change.

Write as if you are speaking to users, not other Knative contributors.
If this
change has no user-visible impact, no release note is needed.
-->

```release-note

```


**Docs**

<!--
:book: If this change has user-visible impact, link to an issue or PR in
https://github.com/knative/docs.
-->
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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants