Skip to content

ApiServerSource Updates#1864

Merged
knative-prow-robot merged 5 commits into
knative:masterfrom
nachocano:apiserversource
Sep 11, 2019
Merged

ApiServerSource Updates#1864
knative-prow-robot merged 5 commits into
knative:masterfrom
nachocano:apiserversource

Conversation

@nachocano
Copy link
Copy Markdown
Contributor

@nachocano nachocano commented Sep 11, 2019

Helps with #1693

Proposed Changes

  • Updating pkg to leverage up-streamed changes for metrics.
  • Removing unnecessary code.
  • Updating UTs to check whether the event count matches the expected one or not.

Release Note

NONE

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Sep 11, 2019
@knative-prow-robot knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 11, 2019
@nachocano
Copy link
Copy Markdown
Contributor Author

nachocano commented Sep 11, 2019

/assign @Harwayne
fyi @sayanh

This was referenced Sep 11, 2019
Closed
Merged
matzew
matzew reviewed Sep 11, 2019
View reviewed changes
Comment thread cmd/apiserver_receive_adapter/main.go
Harwayne
Harwayne reviewed Sep 11, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@Harwayne Harwayne left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold

Holding in case you want to do even better and actually assert the tags of the unit tested metrics. If you don't want to do so now, make an issue and remove the hold.

Comment thread pkg/adapter/apiserver/adapter_test.go Outdated
Comment thread pkg/adapter/apiserver/adapter_test.go
Comment thread pkg/adapter/apiserver/ref_test.go
@knative-prow-robot knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Sep 11, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Sep 11, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Harwayne, nachocano

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 11, 2019
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2019
@nachocano
Copy link
Copy Markdown
Contributor Author

nachocano commented Sep 11, 2019

Holding in case you want to do even better and actually assert the tags of the unit tested metrics. If you don't want to do so now, make an issue and remove the hold.

If you are OK with it, then let's do it later on... as we might need another refactor. We currently don't have access to the tags we create inside the method... I can take a closer look at that when refactoring broker/trigger UTs

@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Sep 11, 2019

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/adapter/apiserver/adapter.go 67.6% 85.3% 17.6

@nachocano nachocano mentioned this pull request Sep 11, 2019
Closed
@nachocano
Copy link
Copy Markdown
Contributor Author

nachocano commented Sep 11, 2019

Holding in case you want to do even better and actually assert the tags of the unit tested metrics. If you don't want to do so now, make an issue and remove the hold.

If you are OK with it, then let's do it later on... as we might need another refactor. We currently don't have access to the tags we create inside the method... I can take a closer look at that when refactoring broker/trigger UTs

Issue here: #1871

@nachocano
Copy link
Copy Markdown
Contributor Author

nachocano commented Sep 11, 2019

/hold cancel

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2019
@akashrv
Copy link
Copy Markdown
Contributor

akashrv commented Sep 11, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2019
@knative-prow-robot knative-prow-robot merged commit 3794bdb into knative:master Sep 11, 2019
@matzew matzew mentioned this pull request Oct 10, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matzew matzew matzew left review comments

@evankanderson evankanderson Awaiting requested review from evankanderson

+1 more reviewer

@Harwayne Harwayne Harwayne left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Harwayne Harwayne

@akashrv akashrv

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@nachocano @knative-prow-robot @knative-metrics-robot @akashrv @matzew @Harwayne @googlebot