Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

mt github receive adapter #1297

Merged
merged 9 commits into from
Jun 25, 2020
Merged

mt github receive adapter #1297

merged 9 commits into from
Jun 25, 2020

Conversation

lionelvillard
Copy link
Member

@lionelvillard lionelvillard commented Jun 3, 2020 •
edited
Loading

Fixes #

Proposed Changes

  • Add new github multi-tenant receive adapter
  • Add new release asset mt-github.yaml
  • Create Webhook of the form http://<name>.<ns>.<domain> (temporary. See Vanity GitHub Webhook URL #1171)

Release Note

Add new multi-tenant GitHubSource adapter.  See the documentation for more information
on how to enable it.

Docs

knative/docs#2544

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 3, 2020
@knative-prow-robot knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 3, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lionelvillard

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2020
mattmoor
mattmoor reviewed Jun 3, 2020
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

@lionelvillard lionelvillard mentioned this pull request Jun 3, 2020
Closed
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2020
mattmoor
mattmoor reviewed Jun 3, 2020
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

mattmoor
mattmoor reviewed Jun 3, 2020
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

github/cmd/mt_receive_adapter/main.go
package main

import (
"flag"
Copy link
Member

Choose a reason for hiding this comment

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

Format Go code:

Suggested change
"flag"
"flag"

@lionelvillard lionelvillard mentioned this pull request Jun 3, 2020
Closed
@lionelvillard lionelvillard changed the title WIP: mt github receive adapter mt github receive adapter Jun 3, 2020
@knative-prow-robot knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 3, 2020
@lionelvillard
Copy link
Member Author

lionelvillard commented Jun 3, 2020 •
edited
Loading

This is ready to go. There are missing bits such as logging and metrics. This can be addressed in subsequent PRs (including go coverage).

The existing single-tenant receive adapter still exists and can be installed the same way as before.

@aliok
Copy link
Member

aliok commented Jun 4, 2020

/assign

@lionelvillard
Copy link
Member Author

lionelvillard commented Jun 5, 2020 •
edited
Loading

@aliok did you have a chance to look at this?

@aliok
Copy link
Member

aliok commented Jun 5, 2020

/unassign

@lionelvillard I am sorry I couldn't.
let me unassign myself

@slinkydeveloper
Copy link
Contributor

/assign

@aslom
Copy link
Member

aslom commented Jun 18, 2020

/assign

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 18, 2020
@knative-prow-robot knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 18, 2020
@aslom
Copy link
Member

aslom commented Jun 25, 2020

I have tested code by deploying github and mt-github versions and verifying that github adapter in knative-sources is used.

@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2020
@aslom
Copy link
Member

aslom commented Jun 25, 2020 •
edited
Loading

After testing and review

/lgtm

@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
github/pkg/adapter/adapter.go 88.2% 85.0% -3.2
github/pkg/mtadapter/adapter.go Do not exist 88.2%
github/pkg/mtadapter/controller.go Do not exist 100.0%
github/pkg/mtadapter/githubsource.go Do not exist 86.7%
github/pkg/mtadapter/router.go Do not exist 100.0%
github/pkg/reconciler/source/controller.go Do not exist 6.2%
github/pkg/reconciler/source/fake_webhook_client.go Do not exist 100.0%
github/pkg/reconciler/source/githubsource.go Do not exist 0.0%

@knative-prow-robot
Copy link
Contributor

@lionelvillard: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-knative-eventing-contrib-go-coverage 79858f8 link /test pull-knative-eventing-contrib-go-coverage

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@lionelvillard
Copy link
Member Author

/unhold

got another review
thanks @aslom!!

@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 Jun 25, 2020
@lionelvillard
Copy link
Member Author

/lgtm

@knative-prow-robot
Copy link
Contributor

@lionelvillard: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

@nachocano
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2020
@knative-prow-robot knative-prow-robot merged commit 0f18dc7 into knative:master Jun 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mattmoor mattmoor mattmoor left review comments

@coryrc coryrc Awaiting requested review from coryrc

@grantr grantr Awaiting requested review from grantr

@lberk lberk Awaiting requested review from lberk

@rootfs rootfs Awaiting requested review from rootfs

@slinkydeveloper slinkydeveloper Awaiting requested review from slinkydeveloper

Assignees

@aslom aslom

@nachocano nachocano

@slinkydeveloper slinkydeveloper

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@lionelvillard @knative-prow-robot @aliok @slinkydeveloper @aslom @knative-metrics-robot @nachocano @mattmoor @googlebot