Skip to content

Types for v1 duck#3350

Merged
knative-prow-robot merged 9 commits into
knative:masterfrom
nlopezgi:issue3335
Jun 22, 2020
Merged

Types for v1 duck#3350
knative-prow-robot merged 9 commits into
knative:masterfrom
nlopezgi:issue3335

Conversation

@nlopezgi
Copy link
Copy Markdown
Contributor

@nlopezgi nlopezgi commented Jun 19, 2020

Part of #3336

Proposed Changes

  • Add types for Add V1 api for duck.* - a prerequisite for doing messaging (will work on messaging afterwards)

@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 19, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 19, 2020
@knative-prow-robot knative-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 19, 2020
@matzew
Copy link
Copy Markdown
Member

matzew commented Jun 19, 2020

Not sure it fixes #3336 - it's part of that ticket

@nlopezgi
Copy link
Copy Markdown
Contributor Author

Not sure it fixes #3336 - it's part of that ticket

Thanks for noting, updated comment

@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 and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 19, 2020
@nlopezgi nlopezgi force-pushed the issue3335 branch 3 times, most recently from aa44a09 to f473eee Compare June 19, 2020 16:11
@lionelvillard lionelvillard mentioned this pull request Jun 19, 2020
@nlopezgi nlopezgi changed the title [WIP] draft types for v1 duck and messaging [WIP] draft types for v1 duck Jun 22, 2020
@nlopezgi nlopezgi changed the title [WIP] draft types for v1 duck Types for v1 duck Jun 22, 2020
@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 22, 2020
@nlopezgi
Copy link
Copy Markdown
Contributor Author

This is now ready for review. fyi @matzew @vaikas

@matzew
Copy link
Copy Markdown
Member

matzew commented Jun 22, 2020

@nlopezgi Please run hack/update-codegen.sh

@nlopezgi
Copy link
Copy Markdown
Contributor Author

@nlopezgi Please run hack/update-codegen.sh

thanks, running it right now

@nlopezgi
Copy link
Copy Markdown
Contributor Author

@nlopezgi Please run hack/update-codegen.sh

Had to rebase as CI still complained after I ran hack/update-codegen.sh. Will ping here once build-tests are passing

@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented Jun 22, 2020

/lgtm
/approve

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 22, 2020
@nlopezgi
Copy link
Copy Markdown
Contributor Author

ugh, ran ./hack/update-deps.sh --upgrade && ./hack/update-codegen.sh and CI is still complaining, I think I might have a tool used by update-codegen stuck in an older version, not sure which one though. Any advice is appreciated.

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2020
@nlopezgi
Copy link
Copy Markdown
Contributor Author

updated godep on my dev machine, lets see if that fixes the issue.

@nlopezgi
Copy link
Copy Markdown
Contributor Author

Still failing on CI, I ran verify-codegen locally and it succeeds, so there has to be some tool that I have in a different version than prow for this to fail. And its not ko or godep, as I just verified those two. Any advice is appreciated.

@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented Jun 22, 2020

Yeah, that's weird as it still is saying this:

/home/prow/go/src/knative.dev/eventing is out of date. Please run hack/update-codegen.sh

Of course, this seems a bit odd?:

./test/../vendor/knative.dev/test-infra/scripts/library.sh: line 534: /go/bin/kntest: Argument list too long

@matzew
Copy link
Copy Markdown
Member

matzew commented Jun 22, 2020 via email

@knative-metrics-robot
Copy link
Copy Markdown

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/duck/v1/channelable_types.go Do not exist 87.5%
pkg/apis/duck/v1/delivery_conversion.go Do not exist 100.0%
pkg/apis/duck/v1/delivery_types.go Do not exist 92.3%
pkg/apis/duck/v1/subscribable_types.go Do not exist 100.0%
pkg/apis/duck/v1/subscribable_types_conversion.go Do not exist 100.0%
pkg/apis/duck/v1beta1/delivery_conversion.go 100.0% 68.8% -31.2

@vaikas
Copy link
Copy Markdown
Contributor

vaikas commented Jun 22, 2020

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nlopezgi, 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

@nlopezgi
Copy link
Copy Markdown
Contributor Author

Ok, looks like build tests finally passed. Thanks for the advice on how to fix update-deps, I honestly have no idea what is wrong with my setup, but this is now ready.

@knative-prow-robot knative-prow-robot merged commit a6b7494 into knative:master Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

6 participants