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

Subscription v1<>v1beta conversion #3421

Merged
merged 3 commits into from
Jun 26, 2020
Merged

Subscription v1<>v1beta conversion #3421

merged 3 commits into from
Jun 26, 2020

Conversation

aliok
Copy link
Member

@aliok aliok commented Jun 26, 2020 •
edited
Loading

Part of #3336

Proposed Changes

  • Conversion for subscription v1<>v1beta1

Release Note

Docs

@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 26, 2020
@knative-prow-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 26, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 26, 2020
mattmoor
mattmoor reviewed Jun 26, 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 -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

mattmoor
mattmoor reviewed Jun 26, 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 -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

Unable to apply comments to the following files:

  • pkg/apis/eventing/v1beta1/broker_conversion_test.go
  • pkg/apis/eventing/v1beta1/trigger_conversion_test.go

@aliok aliok changed the title [WIP] Subscription v1<>v1beta conversion Subscription v1<>v1beta conversion Jun 26, 2020
@aliok aliok marked this pull request as ready for review June 26, 2020 08:09
@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 26, 2020
@aliok
Copy link
Member Author

aliok commented Jun 26, 2020

/assign @nlopezgi

@aliok aliok force-pushed the subscription_v1beta_v1_conversion branch from 6ae9bcc to 37fa48b Compare June 26, 2020 08:36
@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
pkg/apis/messaging/v1beta1/subscription_conversion.go 100.0% 93.3% -6.7

pierDipi
pierDipi reviewed Jun 26, 2020
View reviewed changes
pkg/apis/messaging/v1beta1/subscription_conversion_test.go
"k8s.io/utils/pointer"
eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1"
duckv1beta1 "knative.dev/eventing/pkg/apis/duck/v1beta1"
v1 "knative.dev/eventing/pkg/apis/messaging/v1"
Copy link
Member

Choose a reason for hiding this comment

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

messagingv1

Copy link
Member

Choose a reason for hiding this comment

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

or remove the renaming

Copy link
Member Author

Choose a reason for hiding this comment

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

If you don't feel too strong about it, let's leave it as v1.
This test is about converting subscriptions from v1beta1 to v1. Name v1 is more understandable IMO as we're already in messaging v1beta1 package.

Copy link
Member Author

@aliok aliok Jun 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

matmoor's bot suggested to add v1, so I added the name :)

Copy link
Member

Choose a reason for hiding this comment

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

Agree, but then we don't need to rename it, right?

Copy link
Member

Choose a reason for hiding this comment

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

Now, I saw a similar pattern for the other conversion and there we don't include messaging* or eventing*, sorry for the noise :)

pkg/apis/messaging/v1beta1/subscription_conversion.go
duckv1 "knative.dev/eventing/pkg/apis/duck/v1"
duckv1beta1 "knative.dev/eventing/pkg/apis/duck/v1beta1"

v1 "knative.dev/eventing/pkg/apis/messaging/v1"
Copy link
Member

Choose a reason for hiding this comment

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

messagingv1

pierDipi
pierDipi approved these changes Jun 26, 2020
View reviewed changes
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2020
@lionelvillard
Copy link
Member

/approve

The webhook needs to be updated (separate PR)

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, lionelvillard, pierDipi

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 26, 2020
@knative-prow-robot knative-prow-robot merged commit a11e307 into knative:master Jun 26, 2020
vaikas pushed a commit to vaikas/eventing that referenced this pull request Jun 28, 2020
@aliok @vaikas
Subscription v1<>v1beta conversion (knative#3421)
6ddf113 
* Subscription v1<>v1beta conversion and simple tests

* Subscription v1<>v1beta conversion roundtrip tests

* Formatting stuff
@nlopezgi nlopezgi mentioned this pull request Jun 29, 2020
Merged
@pierDipi pierDipi mentioned this pull request Sep 22, 2020
Merged
@aliok aliok deleted the subscription_v1beta_v1_conversion branch April 28, 2021 06:56
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

@mattmoor mattmoor mattmoor left review comments

@pierDipi pierDipi pierDipi approved these changes

@nachocano nachocano Awaiting requested review from nachocano

@slinkydeveloper slinkydeveloper Awaiting requested review from slinkydeveloper

Assignees

@nlopezgi nlopezgi

@pierDipi pierDipi

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@aliok @knative-prow-robot @knative-metrics-robot @lionelvillard @matzew @mattmoor @pierDipi @nlopezgi @googlebot