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.

Sign up for GitHub

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

Jump to bottom

Apiseversource reconciler and e2e for v1beta1 #3627

Merged
merged 10 commits into from
Jul 17, 2020
Merged

Apiseversource reconciler and e2e for v1beta1 #3627

merged 10 commits into from
Jul 17, 2020

Conversation

capri-xiyue
Copy link
Contributor

@capri-xiyue capri-xiyue commented Jul 17, 2020 •
edited
Loading

Helps with #3611

Proposed Changes

  • ApiSeverSource reconciler and e2e for v1beta1

Release Note

ApiSeverSource is now in v1beta1

@capri-xiyue capri-xiyue changed the title Apiseversource reconciler and e2e Apiseversource reconciler and e2e for v1beta1 Jul 17, 2020
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jul 17, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 17, 2020
@knative-prow-robot knative-prow-robot added the area/test-and-release Test infrastructure, tests or release label Jul 17, 2020
@capri-xiyue
Copy link
Contributor Author

/assign @nachocano @lionelvillard

@knative-prow-robot knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 17, 2020
@capri-xiyue @mattmoor
Update test/e2e/source_api_server_v1beta1_test.go
c768c32 
Co-authored-by: Matt Moore <mattmoor@vmware.com>
@nachocano
Copy link
Contributor

/lgtm
/approve

Thanks @capri-xiyue !!!
Can you update the Release Notes of this PR saying ApiServerSource is now in v1beta1.
Also create a follow up issue similar to #3615 for 0.18
And the last one will be the documentation PR, similar to knative/docs#2673

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: capri-xiyue, nachocano

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 Jul 17, 2020
@capri-xiyue
Copy link
Contributor Author

/test pull-knative-eventing-integration-tests

1 similar comment
@capri-xiyue
Copy link
Contributor Author

/test pull-knative-eventing-integration-tests

@capri-xiyue capri-xiyue mentioned this pull request Jul 17, 2020
Merged
@capri-xiyue
Copy link
Contributor Author

/lgtm
/approve

Thanks @capri-xiyue !!!
Can you update the Release Notes of this PR saying ApiServerSource is now in v1beta1.
Also create a follow up issue similar to #3615 for 0.18
And the last one will be the documentation PR, similar to knative/docs#2673

done

bharattkukreja
bharattkukreja reviewed Jul 17, 2020
View reviewed changes
pkg/reconciler/testing/v1beta1/listers.go
@@ -180,6 +182,10 @@ func (l *Listers) GetApiServerSourceV1alpha2Lister() sourcev1alpha2listers.ApiSe
return sourcev1alpha2listers.NewApiServerSourceLister(l.indexerFor(&sourcesv1alpha2.ApiServerSource{}))
}

func (l *Listers) GetApiServerSourceV1beta1Lister() sourcev1beta1listers.ApiServerSourceLister {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be added to pkg/testing/listers.go as well?

Copy link
Contributor Author

@capri-xiyue capri-xiyue Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I created another follow up issue here: #3629
I think ideally we should only have one lister.go under pkg/testing or different versions of lister under different versions of package. I added it here because that's what v1alpha2 ApiSeverSource used. I think for container source, you can add it in either one now. Later, we can clean it up for all resources in #3629

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that makes sense. I was wondering the same thing. Container source test uses testing/listers.go so I'll add it there for now. But thanks for creating the issue!

Copy link
Contributor Author

@capri-xiyue capri-xiyue Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I took another look at the codebase. I think pkg/testing/v1 is for v1 resources, pkg/testing/v1beta1 is for v1beta1 resources. I think it's correct that I put lister under pkg/testing/v1beta1.
But unlike broker, sources use different code style. They don't create resources under specific version package.
I think we need another PR to clean it up

@knative-test-reporter-robot
Copy link

The following jobs failed:

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

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

test/e2e.TestChannelBasedBrokerWithManyTriggers
test/e2e.TestChannelBasedBrokerWithManyTriggers/InMemoryChannel-messaging.knative.dev/v1beta1
test/e2e.TestChannelBasedBrokerWithManyTriggers/InMemoryChannel-messaging.knative.dev/v1beta1/test_default_broker_with_many_attribute_triggers

@capri-xiyue
Copy link
Contributor Author

/test pull-knative-eventing-integration-tests

@knative-prow-robot knative-prow-robot merged commit 5632b09 into knative:master Jul 17, 2020
@capri-xiyue
Copy link
Contributor Author

It looks like currently for the code style of sources e2e tests, we duplicate a lot of code.
Created follow up issue here: #3631
FYI: @nachocano @bharattkukreja

@capri-xiyue capri-xiyue mentioned this pull request Sep 30, 2020
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bharattkukreja bharattkukreja bharattkukreja left review comments

@nachocano nachocano nachocano left review comments

@mattmoor mattmoor mattmoor left review comments

@chaodaiG chaodaiG Awaiting requested review from chaodaiG

@yolocs yolocs Awaiting requested review from yolocs

Assignees

@nachocano nachocano

@lionelvillard lionelvillard

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/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.
8 participants
@capri-xiyue @nachocano @knative-prow-robot @knative-test-reporter-robot @mattmoor @bharattkukreja @googlebot @lionelvillard