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

changed reconciler, UT and e2e to be v1beta1 #1264

Merged
merged 16 commits into from
Jun 12, 2020

Conversation

capri-xiyue
Copy link
Contributor

@capri-xiyue capri-xiyue commented Jun 10, 2020

Fixes #1168
Fixes #1211

Proposed Changes

  • 🎁 Moving CloudBuildSource to v1beta1.
  • 🧽 All reconcilers now reconcile v1beta1 resources.
  • 🧽 All UTs and E2E tests use v1beta1 resources

Release Note

Moving CloudBuildSource to v1beta1.

Docs

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@googlebot googlebot added the cla: yes (override cla status due to multiple authors bug) label Jun 10, 2020
@nachocano
Copy link
Member

I know is WIP, but do you think is possible to separate the CloudBuildSource move to v1beta1 in another PR? As that one is not high priority, and I think we don't have E2E tests for it yet?

Copy link
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.

It looks like this PR is doing multiple things. I recommend breaking it up:

@capri-xiyue capri-xiyue changed the title [WIP]changed channel to v1beta1 and added cloudbuildsource in v1beta1 [WIP]changed reconciler, UT and e2e to be v1beta1 Jun 11, 2020
@knative-test-reporter-robot

The following jobs failed:

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

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

github.com/google/knative-gcp/test/e2e.[build failed]

@knative-test-reporter-robot

The following jobs failed:

Test name Triggers Retries
pull-google-knative-gcp-wi-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-google-knative-gcp-wi-tests:

github.com/google/knative-gcp/test/e2e.[build failed]

@capri-xiyue
Copy link
Contributor Author

capri-xiyue commented Jun 11, 2020

It looks like this PR is doing multiple things. I recommend breaking it up:

I didn't realize I need to create CloudBuildSource v1beta1 version until I ran into build failure.
All sources share the same test helper methods(like pullsubscriptions and topics)
If we don'r push CloudBuildSource to v1beta1, we need to have two versions of test helper methods, one for v1alpha1 and the other for v1beta1.

I will try to see whether it's easy to break it into two PRs

@capri-xiyue
Copy link
Contributor Author

/retest

@capri-xiyue
Copy link
Contributor Author

/test pull-google-knative-gcp-build-tests

@capri-xiyue
Copy link
Contributor Author

/test pull-google-knative-gcp-unit-tests

1 similar comment
@capri-xiyue
Copy link
Contributor Author

/test pull-google-knative-gcp-unit-tests

@capri-xiyue
Copy link
Contributor Author

capri-xiyue commented Jun 11, 2020

All E2E tests related to pubsub channel broke again.
I will try to see if I can reproduce the same failure in local

@capri-xiyue capri-xiyue changed the title [WIP]changed reconciler, UT and e2e to be v1beta1 changed reconciler, UT and e2e to be v1beta1 Jun 11, 2020
@nachocano
Copy link
Member

Can you update the description and release notes of the PR? We auto-generate the release notes based on that, so it's better to include this...

I'm willing to let this in because we are kind of in a hurry and there are other P1s. I don't see a problem as we are not releasing CloudBuildSource for now.
I think next time we can try and break it down into multiple PRs as Adam mentioned.. I understand that it would have meant more work for the UTs though...

If @Harwayne agrees, I think after the comments are resolved, we can get this in asap...

Comment on lines 60 to 65
- name: v1alpha1
served: true
storage: true
- name: v1beta1
served: true
storage: false
Copy link
Member

Choose a reason for hiding this comment

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

@Harwayne shall we change everywhere to?

versions:

  • name: v1alpha1
    served: false
    storage: false
  • name: v1beta1
    served: true
    storage: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I can change it to things as above.
Otherwise, we can't create v1alpha1 resources anymore
It looks like it should be
versions:

  • name: v1alpha1
    served: true
    storage: false
  • name: v1beta1
    served: true
    storage: true

Copy link
Member

Choose a reason for hiding this comment

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

ok, please sync with Adam tomorrow, as he knows better about this... If it passes, I think we can let it in...

@capri-xiyue
Copy link
Contributor Author

/retest

@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/apis/duck/v1beta1/annotations.go 93.2% 77.8% -15.4
pkg/apis/events/v1alpha1/cloudbuildsource_conversion.go Do not exist 100.0%
pkg/apis/events/v1beta1/cloudbuildsource_conversion.go Do not exist 100.0%
pkg/apis/events/v1beta1/cloudbuildsource_defaults.go Do not exist 100.0%
pkg/apis/events/v1beta1/cloudbuildsource_lifecycle.go Do not exist 100.0%
pkg/apis/events/v1beta1/cloudbuildsource_types.go Do not exist 71.4%
pkg/apis/events/v1beta1/cloudbuildsource_validation.go Do not exist 87.5%
pkg/apis/intevents/v1beta1/topic_lifecycle.go 56.7% 54.8% -1.8
pkg/reconciler/messaging/channel/channel.go 78.0% 78.7% 0.7

@nachocano
Copy link
Member

Thanks @capri-xiyue this huuuugeee change!!!

/lgtm

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved cla: yes (override cla status due to multiple authors bug) lgtm size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CloudBuildSource should not have Topic in its spec E2E tests and Reconciler should use v1beta1 APIs
7 participants