Vendor cornice and cornice.ext.swagger

[leplatrem]

In order to remove the number of repositories that we maintain to support https://github.com/mozilla/remote-settings we could vendor and cornice and cornice.ext.swagger.

In the past years, I have laboriously maintained a myriad of repositories by myself. It was not always fun. Many open issues require to dive deeply in code to support specific use-case and corner-cases. It is very time consuming, for very little value for our project. I don't want to do this anymore.

Since Pylons/trypyramid.com#388 we don't recommend using Cornice in your new Pyramid projects.
Cornice is an abstraction layer on top of Pyramid, and with this PR, it becomes an internal module of Kinto.

I am very open to suggestions on how to execute this major move.

• Vendor libs
• Remove dead code (total coverage 97%)
• Import unit tests to fix branching coverage
• Add message/banner on the Cornice and Cornice.ext.swagger repos to official look for maintainers

[leplatrem]

kinto/core/cornice/cors.py                               70     10     40      8    80%   34, 38, 43, 51, 54-56, 103, 107, 109
kinto/core/cornice/errors.py                             24      8      6      2    60%   21, 24, 31-32, 37-40
kinto/core/cornice/pyramidhook.py                       142     16     68     12    85%   74->54, 82->54, 119-122, 145->148, 148->154, 150, 154->exit, 169-170, 210, 213->218, 243, 266->exit, 341-344, 358-361
kinto/core/cornice/renderer.py                           18      3      6      3    75%   30->42, 35-36, 40
kinto/core/cornice/service.py                           232     34    120     21    79%   20, 27, 155, 172, 179-180, 195->191, 199, 227-228, 237, 262-265, 305, 399, 422->424, 458-464, 469, 475, 485->484, 505-508, 518, 532, 537->542, 542->544, 569->571, 573, 574->580, 583, 608, 619->623
kinto/core/cornice/util.py                               43     12      4      1    72%   45-47, 67-69, 73-76, 80-81, 117, 132->exit
kinto/core/cornice/validators/__init__.py                29      4     12      3    83%   37, 39, 49-50
kinto/core/cornice/validators/_colander.py               17      1      6      2    87%   27->30, 31
kinto/core/cornice_swagger/__init__.py                   18      0      0      0   100%
kinto/core/cornice_swagger/converters/__init__.py        10      6      0      0    40%   11-14, 18-21
kinto/core/cornice_swagger/converters/exceptions.py       4      0      0      0   100%
kinto/core/cornice_swagger/converters/parameters.py      49      6     12      5    82%   16, 19, 40-41, 60, 85
kinto/core/cornice_swagger/converters/schema.py         145     18     56     12    81%   16-20, 41->49, 44->46, 46->49, 59, 61, 73->76, 88-92, 112, 116, 135, 241-244
kinto/core/cornice_swagger/swagger.py                   317     58    128     34    77%   74-85, 134, 138, 141->129, 145, 167, 185-190, 231, 239, 243->233, 246->233, 255, 273-278, 397->401, 445->453, 449->448, 453->457, 459-460, 464-465, 469-470, 476-480, 485, 525, 538, 545-546, 554, 558->564, 562, 565, 591-597, 638-641, 643->647, 671-674, 679, 695, 719, 722

[leplatrem]

I have tried to import tests from cornice and cornice.ext.swagger in https://github.com/Kinto/kinto/tree/import-cornice-tests
but been stuck in a rabbit hole..

1. I removed the dead code from cornice (code not leveraged in kinto) ~1000 LOC (see 56c4e7a)
2. The tests have side effects in tests/core/test_openapi.py and tests/openapi/ and it drives me crazy. See also Unit tests should be independent #3401
3. We have branch coverage 100% in Kinto, but we didn't in the cornice and cornice_swagger repos, which means that in order to reach 100% of branch coverage we have to write new tests (time consuming for little value)

I am tempted to propose to leave the PR like this with code coverage ignored in kinto/core/cornice and kinto/core/cornice_swagger. What do you think?

[alexcottner]

Getting some thoughts out, might be slightly messy.

• I think bringing corncie, or at least the parts that kinto is using, into kinto is a good idea.
• Maintaining it as a separate project adds work and you've told people not to use it in new projects. Maybe it's time to update the cornice read-me stating this as well?
• Vendoring a dependency and then cleaning it up like this always involves some additional cleanup work down the road.
• I think this is a reasonable first step.
• The tests having side effects is a bummer. Perhaps we should exclude them from the main test suite and run those tests separately in our CI pipeline for now? We could adjust the make file to do this too.
• I don't think hitting 100% branch coverage for cornice is worth the effort here. Hopefully many of them are unused for kinto as well.

If doing this means you don't have to dedicate more time to a repo that's in maintenance-only mode, let's go for it.

[say-yawn]

I am tempted to propose to leave the PR like this with code coverage ignored in kinto/core/cornice and kinto/core/cornice_swagger. What do you think?

That sounds good to me. Especially if there are tests already in kinto code that utilizes cornice I think it's fine to ignore vendored code for test coverage.

[leplatrem]

Thank you for your inputs here 🙏

Getting some thoughts out, might be slightly messy.

* I think bringing corncie, or at least the parts that kinto is using, into kinto is a good idea.

Great!

* Maintaining it as a separate project adds work and you've told people not to use it in new projects. Maybe it's time to update the cornice read-me stating this as well?

I did. Should we release a new version that emits a warning saying no one is maintaining it anymore?

* Vendoring a dependency and then cleaning it up like this always involves some additional cleanup work down the road.

Shall I include the commit that removes the dead code or not? If we disable coverage, maybe it's not necessary. And if for whatever reason we want to be able to backport patches, it's easier if the codebase does drift too much

* I think this is a reasonable first step.

Great, I'll open a draft PR with the follow-ups

* The tests having side effects is a bummer. Perhaps we should exclude them from the main test suite and run those tests separately in our CI pipeline for now? We could adjust the make file to do this too.

That's a good idea. I want to believe that there is a single place that ramificates into several tests, so maybe with another pair of eyes we can find the culprit

* I don't think hitting 100% branch coverage for cornice is worth the effort here. Hopefully many of them are unused for kinto as well.

If doing this means you don't have to dedicate more time to a repo that's in maintenance-only mode, let's go for it.

Great, ready to review 😎

I am tempted to propose to leave the PR like this with code coverage ignored in kinto/core/cornice and kinto/core/cornice_swagger. What do you think?

That sounds good to me. Especially if there are tests already in kinto code that utilizes cornice I think it's fine to ignore vendored code for test coverage.

Awesome 👌

Thaaanks