Skip to content

move parallel/sequence to flows.knative.dev api group#2156

Merged
knative-prow-robot merged 4 commits into
knative:masterfrom
vaikas:flows.apigroup
Nov 8, 2019
Merged

move parallel/sequence to flows.knative.dev api group#2156
knative-prow-robot merged 4 commits into
knative:masterfrom
vaikas:flows.apigroup

Conversation

@vaikas
Copy link
Copy Markdown
Contributor

@vaikas vaikas commented Nov 7, 2019 •
edited
Loading

Addresses #1913

Proposed Changes

  • move parallel/sequence to flows.knative.dev api group. This is only the types. I'll do controller next.

Release Note

Introduce flows.knative.dev/v1beta1 and move parallel/sequence to it. Types only

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 7, 2019
@knative-prow-robot knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/test-and-release Test infrastructure, tests or release labels Nov 7, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

knative-prow-robot commented Nov 7, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vaikas

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 Nov 7, 2019
matzew
matzew reviewed Nov 7, 2019
View reviewed changes
Comment thread pkg/apis/flows/v1beta1/parallel_types.go Outdated
nachocano
nachocano reviewed Nov 7, 2019
View reviewed changes
Comment thread pkg/apis/flows/register.go Outdated
nachocano
nachocano reviewed Nov 7, 2019
View reviewed changes
Comment thread pkg/apis/flows/v1beta1/doc.go
nachocano
nachocano reviewed Nov 7, 2019
View reviewed changes
Comment thread pkg/apis/flows/v1beta1/parallel_types.go
_ kmeta.OwnerRefable = (*Parallel)(nil)
)

type ParallelSpec struct {
Copy link
Copy Markdown
Contributor

@nachocano nachocano Nov 7, 2019

Choose a reason for hiding this comment

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

is the old v1alpha1 ParallelSpec the same as this one? Should we then inline this one there to remove duplicated code? Same with Sequence? and with their statuses...

Copy link
Copy Markdown
Contributor Author

@vaikas vaikas Nov 7, 2019

Choose a reason for hiding this comment

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

Yeah, so I was hoping to do this in couple of steps:

  1. Get the types moved to the new API group
  2. Create the controller mods
  3. Decide how the migration story goes

Copy link
Copy Markdown
Contributor

@nachocano nachocano Nov 7, 2019

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Copy Markdown
Contributor

@lionelvillard lionelvillard Nov 7, 2019

Choose a reason for hiding this comment

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

have we decided to move Parallel and Sequence to v1beta1? I would prefer to stick with v1alpha1 for now.

Copy link
Copy Markdown
Member

@matzew matzew Nov 8, 2019

Choose a reason for hiding this comment

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

I'd be fine in upgrading those as well ...

@vaikas
Copy link
Copy Markdown
Contributor Author

vaikas commented Nov 7, 2019

@lionelvillard could you take a looksie too?

@lionelvillard
Copy link
Copy Markdown
Contributor

lionelvillard commented Nov 7, 2019

@vaikas did you see my comment about v1alpha1?

matzew
matzew reviewed Nov 8, 2019
View reviewed changes
Comment thread pkg/apis/flows/register.go
package flows

const (
GroupName = "flows.knative.dev"
Copy link
Copy Markdown
Member

@matzew matzew Nov 8, 2019

Choose a reason for hiding this comment

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

register them

https://github.com/knative/eventing/blob/master/cmd/webhook/main.go#L75-L79

?

Copy link
Copy Markdown
Member

@matzew matzew Nov 8, 2019

Choose a reason for hiding this comment

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

I just see that this PR is just types.

@vaikas
Copy link
Copy Markdown
Contributor Author

vaikas commented Nov 8, 2019

sounds like we're not convinced that it's time to hoist these to v1beta1 yet, so I'll revert these to v1alpha1.

@vaikas
Copy link
Copy Markdown
Contributor Author

vaikas commented Nov 8, 2019

/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2019
@matzew
Copy link
Copy Markdown
Member

matzew commented Nov 8, 2019 via email

@vaikas vaikas removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2019
@vaikas
Copy link
Copy Markdown
Contributor Author

vaikas commented Nov 8, 2019

ok, changed to v1alpha1. PTAL.

@knative-metrics-robot
Copy link
Copy Markdown

knative-metrics-robot commented Nov 8, 2019

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/flows/v1alpha1/parallel_defaults.go Do not exist 100.0%
pkg/apis/flows/v1alpha1/parallel_lifecycle.go Do not exist 82.9%
pkg/apis/flows/v1alpha1/parallel_types.go Do not exist 50.0%
pkg/apis/flows/v1alpha1/parallel_validation.go Do not exist 90.5%
pkg/apis/flows/v1alpha1/register.go Do not exist 100.0%
pkg/apis/flows/v1alpha1/sequence_defaults.go Do not exist 100.0%
pkg/apis/flows/v1alpha1/sequence_lifecycle.go Do not exist 78.9%
pkg/apis/flows/v1alpha1/sequence_types.go Do not exist 50.0%
pkg/apis/flows/v1alpha1/sequence_validation.go Do not exist 94.1%

@lionelvillard
Copy link
Copy Markdown
Contributor

lionelvillard commented Nov 8, 2019

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2019
@knative-prow-robot knative-prow-robot merged commit d958442 into knative:master Nov 8, 2019
@vaikas vaikas deleted the flows.apigroup branch November 8, 2019 18:07
@lionelvillard lionelvillard mentioned this pull request Dec 12, 2019
Merged
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

@grantr grantr Awaiting requested review from grantr

@yt3liu yt3liu Awaiting requested review from yt3liu

+2 more reviewers

@nachocano nachocano nachocano left review comments

@lionelvillard lionelvillard lionelvillard left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@vaikas @knative-prow-robot @lionelvillard @matzew @knative-metrics-robot @nachocano @googlebot