Skip to content
This repository has been archived by the owner on Sep 2, 2024. It is now read-only.

move eventing/pkg/apis/duck/v1alpha1 here #430

Merged
merged 5 commits into from
Mar 5, 2021

Conversation

vaikas
Copy link
Contributor

@vaikas vaikas commented Mar 3, 2021

Addresses #426

Proposed Changes

  • This moves the ducks from knative.dev/eventing/pkg/apis/duck/v1alpha1 here

Release Note

 * Note that after the upstream PR: https://github.com/knative/eventing/pull/5005 goes in, old v1alpha1 channels will not be reconciled.

Docs

@vaikas vaikas requested review from a team as code owners March 3, 2021 22:29
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 3, 2021
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 3, 2021
@codecov
Copy link

codecov bot commented Mar 3, 2021

Codecov Report

Merging #430 (858bb23) into main (ee7edfd) will increase coverage by 0.75%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #430      +/-   ##
==========================================
+ Coverage   73.09%   73.84%   +0.75%     
==========================================
  Files         129      135       +6     
  Lines        5003     5323     +320     
==========================================
+ Hits         3657     3931     +274     
- Misses       1116     1146      +30     
- Partials      230      246      +16     
Impacted Files Coverage Δ
...g/apis/duck/v1alpha1/channelable_combined_types.go 98.55% <ø> (ø)
pkg/apis/duck/v1alpha1/channelable_types.go 98.14% <ø> (ø)
pkg/apis/duck/v1alpha1/subscribable_types.go 100.00% <ø> (ø)
...pis/duck/v1alpha1/subscribable_types_conversion.go 79.48% <ø> (ø)
...pis/messaging/v1alpha1/kafka_channel_conversion.go 92.15% <ø> (ø)
pkg/apis/messaging/v1alpha1/kafka_channel_types.go 100.00% <ø> (ø)
pkg/apis/duck/v1alpha1/placement_types.go 0.00% <0.00%> (ø)
pkg/apis/duck/v1alpha1/register.go 0.00% <0.00%> (ø)
... and 2 more

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 ee7edfd...3668b8e. Read the comment docs.

Base automatically changed from master to main March 4, 2021 16:25
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2021
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2021
@vaikas
Copy link
Contributor Author

vaikas commented Mar 4, 2021

So, as expected the v1alpha1 tests are now failing.

@pierDipi
Copy link
Member

pierDipi commented Mar 4, 2021

I can't think of any other solution.

I guess during a "pre-upgrade step" we have to:

  • change the stored version to v1beta1
  • stop serving v1alpha1

and there no way to downgrade

Such pre-upgrade has to happen before upgrading eventing.

/cc @matzew @travis-minke-sap

@pierDipi
Copy link
Member

pierDipi commented Mar 4, 2021

In this PR we should remove v1alpha1.

@vaikas
Copy link
Contributor Author

vaikas commented Mar 5, 2021

@pierDipi would you like me to make the PR that removes the v1alpha1 as part of this PR? If that's the case, upgrade will not be easily done (not even sure how) since the storage upgrade scripts reads them in at v1alpha1 and converts. I could stop serving v1alpha1 if that's what you'd like. But I think I personally would split the PRs into this, then the next and upgrade separate ones for smaller chunks. Thoughts?

@pierDipi
Copy link
Member

pierDipi commented Mar 5, 2021

If you can make tests work in smaller chunks, I prefer smaller chunks, but at this point I guess v1alpha1 with the latest eventing just doesn't work, that's the reason I was suggesting to remove it, or am I wrong?

For the upgrade part, I'm thinking to a multi step pre-upgrade with multiple jobs, or do you a better solution to give users an upgrade path?

@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/apis/duck/v1alpha1/channelable_combined_types.go Do not exist 90.9%
pkg/apis/duck/v1alpha1/channelable_types.go Do not exist 87.5%
pkg/apis/duck/v1alpha1/subscribable_types.go Do not exist 100.0%
pkg/apis/duck/v1alpha1/subscribable_types_conversion.go Do not exist 86.4%

@vaikas
Copy link
Contributor Author

vaikas commented Mar 5, 2021

@pierDipi ok, I disabled tests with v1alpha1 and everything passes. So, I think we should get this in, and start working on the upgrade path, thoughts?

Copy link
Contributor

@travis-minke-sap travis-minke-sap left a comment

Choose a reason for hiding this comment

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

looks like a good stepping stone ; )

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: travis-minke-sap

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 Mar 5, 2021
@knative-prow-robot knative-prow-robot merged commit c248933 into knative-extensions:main Mar 5, 2021
@vaikas vaikas deleted the ducks branch March 5, 2021 19:34
matzew added a commit to matzew/eventing-kafka that referenced this pull request Nov 12, 2021
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants