Skip to content

Subscription v1<>v1beta conversion#3421

Merged
knative-prow-robot merged 3 commits into
knative:masterfrom
aliok:subscription_v1beta_v1_conversion
Jun 26, 2020
Merged

Subscription v1<>v1beta conversion#3421
knative-prow-robot merged 3 commits into
knative:masterfrom
aliok:subscription_v1beta_v1_conversion

Conversation

@aliok
Copy link
Copy Markdown
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
Copy Markdown
Contributor

knative-prow-robot commented Jun 26, 2020

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

aliok commented Jun 26, 2020

/assign @nlopezgi

aliok
aliok commented Jun 26, 2020
View reviewed changes
Comment thread pkg/apis/eventing/v1beta1/broker_conversion_test.go Outdated
@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
Copy Markdown

knative-metrics-robot commented Jun 26, 2020

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
Comment thread 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
Copy Markdown
Member

@pierDipi pierDipi Jun 26, 2020

Choose a reason for hiding this comment

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

messagingv1

Copy link
Copy Markdown
Member

@pierDipi pierDipi Jun 26, 2020

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
Copy Markdown
Member Author

@aliok aliok Jun 26, 2020

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
Copy Markdown
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
Copy Markdown
Member

@pierDipi pierDipi Jun 26, 2020

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
Copy Markdown
Member

@pierDipi pierDipi Jun 26, 2020

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 :)

Comment thread 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
Copy Markdown
Member

@pierDipi pierDipi Jun 26, 2020

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
Copy Markdown
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
Copy Markdown
Contributor

lionelvillard commented Jun 26, 2020

/approve

The webhook needs to be updated (separate PR)

@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Jun 26, 2020

[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

Details 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

@pierDipi pierDipi pierDipi approved these changes

@nachocano nachocano Awaiting requested review from nachocano

@slinkydeveloper slinkydeveloper Awaiting requested review from slinkydeveloper

+1 more reviewer

@mattmoor mattmoor mattmoor left review comments

Reviewers whose approvals may not affect merge requirements

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