Replace the Knative API dependencies with duck types - #6851
michalvavrik wants to merge 3 commits into
Conversation
squakez
left a comment
There was a problem hiding this comment.
Thanks a lot for the work. A few comments about the changes:
- we can remove the mention to the knative serving version and the script that regen it
- I don't see a strong reason to test the duck types, they are even skipped from coverage
- I am not sure we need the entire API set. We should probably only identify those object that we use along the code and the related fields used.
- I realize we need to restrict the work on serving API only. Eventing is deprecated on our side (just added a note to the issue)
- Nice to see additional tests adding coverage beside the API ones, we can keep them
Overall it seems complete, we can make some cleaning to reduce the size of the API to the real code usage and the duck types unit tests. We can even keep eventing now that the work is done.
|
✔️ Unit test coverage report - coverage increased from 63.8% to 64.1% (+0.3%) |
|
I think we can merge this one and work on follow up issues to polish the pending points identified during the review, or do you prefer tackling them before? |
I prepared changes you asked for yesterday, but I need to finish something else now and go over it again now that I am not tired. I'll push it as a separate commit later today and you will be able to decide if you want this new version or we can drop it and merge the original commit (or I'll address more comments of course). |
5fdbba1 to
5e7db91
Compare
|
There were more things that I think could be removed (Broker and Channel types and Knative Service status.url, and address). I didn't remove them because I can see preexisting tests using them. |
Thanks. Yeah, anything that is not directly used by our code can be removed. In this way we reduce the maintenance and any possible future API change to the bare minimum. Feel free to give it another shot or, if checks pass we can merge and then work on follow up issues. |
|
✔️ Unit test coverage report - coverage increased from 63.8% to 64.1% (+0.3%) |
|
Sorry, bit busy, I'll try to address it tomorrow. |
ccbab94 to
9ab2459
Compare
|
✔️ Unit test coverage report - coverage increased from 64.4% to 64.8% (+0.4%) |
|
Oh merge conflict for go.sum again, I thought I resolved one yesterday. I'll resolve it. |
Signed-off-by: Michal Vavřík <dev@michalvavrik.net>
Signed-off-by: Michal Vavřík <dev@michalvavrik.net>
Signed-off-by: Michal Vavřík <dev@michalvavrik.net>
9ab2459 to
70689d0
Compare
|
✔️ Unit test coverage report - coverage increased from 64.4% to 64.8% (+0.4%) |
1.20.1which is what we test and is API of a sort, but isn't called "Knative API".