|
Description
Tomás Cohen Arazi (tcohen)
2021-05-18 12:33:28 UTC
Created attachment 121110 [details] [review] Bug 28370: Add missing additionalProperties in spec This patch adds missing attribute to the object definitions in the spec. We wrongly supposed it was the default, but it proved not. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1 => SUCCESS: Tests pass, nothing breaks! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 121134 [details] [review] Bug 28370: Add missing additionalProperties in spec This patch adds missing attribute to the object definitions in the spec. We wrongly supposed it was the default, but it proved not. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1 => SUCCESS: Tests pass, nothing breaks! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 121232 [details] [review] Bug 28370: Add missing additionalProperties in spec This patch adds missing attribute to the object definitions in the spec. We wrongly supposed it was the default, but it proved not. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1 => SUCCESS: Tests pass, nothing breaks! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Good Call, Should we also add an explicit coding guideline for this perhaps.. and a QA script catch to spot removals of the line in api/swagger/*.json files or lack of it in new files in that directory? Passing QA Created attachment 121270 [details] [review] Bug 28370: Add tests Those tests are failing, can you have a look please? 1. They caught 3 missing additionalProperties in paths/* 2. It seems that we should ignore the error.json and open a separate bug report Created attachment 121284 [details] [review] Bug 28370: Add missing additionalProperties in spec This patch adds missing attribute to the object definitions in the spec. We wrongly supposed it was the default, but it proved not. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1 => SUCCESS: Tests pass, nothing breaks! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 121285 [details] [review] Bug 28370: Add tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 121286 [details] [review] Bug 28370: Rewrite tests This patch rewrites the tests, and also doesn't enforce additionalProperties to be false. We could need it, and there are routes that need to be reviewed that would break otherwise now: error.json, for example, is used everywhere, and some routes add some payload to it. The main change this patch introduces is using the resolved spec instead of finding each .json file, which wasn't even correct as we have .yaml files already. Parameters and responses are tested. To test: 1. Apply this patch 2. Run: $ kshell k$ prove xt/api.t => SUCCESS: Tests fail! A nice report on the failures is printed Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 121287 [details] [review] Bug 28370: (follow-up) More cases Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 121289 [details] [review] Bug 28370: Fix invalid responses in /patrons This response codes are not actually used. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Bug 28424 fixes the remaining issue. Great teamwork, the test looks a solid improvement. t/db_dependent/api/v1/holds.t tests are failing. Didn't we decide to fix error.json separately, because of the "conflict" param that was not in the spec? bug 28020 (In reply to Jonathan Druart from comment #14) > t/db_dependent/api/v1/holds.t tests are failing. > > Didn't we decide to fix error.json separately, because of the "conflict" > param that was not in the spec? bug 28020 You are missing bug 28369 on your tree. (In reply to Jonathan Druart from comment #14) > Didn't we decide to fix error.json separately, because of the "conflict" > param that was not in the spec? bug 28020 Do you think we should rush about it? It felt like some coordinated teamwork is needed to think about it and propose something sound. Specially when it comes to a generic payload schema. I allowed the extra attributes in error.json as a transitional step as we are a few days away from the release. Can happily work on a better solution on that bug. (In reply to Tomás Cohen Arazi from comment #16) > (In reply to Jonathan Druart from comment #14) > > Didn't we decide to fix error.json separately, because of the "conflict" > > param that was not in the spec? bug 28020 > > Do you think we should rush about it? No, definitely not. I saw you changed it and so that the tests were failing because of that. Wrong assumption! Pushed to master for 21.05, thanks to everybody involved! Depends on Bug 28272 not in 20.11.x |