Make content-type response checks on '/versions` endpoint optional #670

ml-evs · 2021-01-11T15:10:33Z

Closes #668 and closes #669.

Fix misleading error message on /versions endpoint validation.
Make checks on the header=present and media type parameters of the Content-Type header of /versions completely optional.

codecov · 2021-01-11T15:32:03Z

Codecov Report

Merging #670 (fe9a910) into master (5fb7590) will increase coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #670      +/-   ##
==========================================
+ Coverage   93.30%   93.58%   +0.28%     
==========================================
  Files          61       61              
  Lines        3300     3306       +6     
==========================================
+ Hits         3079     3094      +15     
+ Misses        221      212       -9

Flag	Coverage Δ
project	`93.58% <100.00%> (+0.28%)`	⬆️
validator	`66.00% <81.25%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
optimade/validator/validator.py	`84.35% <100.00%> (+2.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fb7590...fe9a910. Read the comment docs.

ml-evs · 2021-01-11T15:34:12Z

Any objection to me using this PR to disable the codecov status checks appearing in GH actions? We'd still get the coverage report as comment, which is more useful.

It's annoying seeing the ❌ on the main repo page for 0.005% coverage drops, I don't think they ever fixed the problems with the threshold parameter in codecov (namely that it doesn't work).

CasperWA

Looks alright. Any way to test it?

Also, the two different parts of this PR (validator fix and codecov change) should be merged as a separate commit each.

The suggested changes here are not critical, and even if rejected it can still be merged, hence the approval. If it's updated, I'll approve right away.

optimade/validator/validator.py

ml-evs · 2021-01-11T16:58:57Z

Looks alright. Any way to test it?

Well it now passes for the netlify preview (showing the optional failure) and still passes on all implementations that already had the headers, so think we're okay.

CasperWA · 2021-01-11T17:00:56Z

Looks alright. Any way to test it?

Well it now passes for the netlify preview (showing the optional failure) and still passes on all implementations that already had the headers, so think we're okay.

Ah you ran it locally for the providers PR? Great! I mean.. it's not a real "test" ... but it'll do ;)

CasperWA · 2021-01-11T17:04:42Z

You can merge from the CLI (using --ff-only) or rebase+merge, but I think you might lose the commit verification? No need to create a merge commit.

ml-evs · 2021-01-11T17:05:35Z

Panic reverted this to a draft, following the discussion:

#669 (comment)

ml-evs · 2021-01-11T17:28:11Z

Right, reworked it slightly to allow for this mixed requirement... hopefully no major issues!

ml-evs · 2021-01-15T14:26:22Z

Any objection to me using this PR to disable the codecov status checks appearing in GH actions? We'd still get the coverage report as comment, which is more useful.

It's annoying seeing the x on the main repo page for 0.005% coverage drops, I don't think they ever fixed the problems with the threshold parameter in codecov (namely that it doesn't work).

Removing the codecov changes here as #672 fixes the threshold for GH status checks

CasperWA

Very minor clarifications of error messages.
Still missing a test 😅 (can be merged without, no worries).

optimade/validator/validator.py

ml-evs · 2021-01-15T16:57:22Z

Still missing a test sweat_smile (can be merged without, no worries).

I was actually writing one before I was rudely interrupted by a meeting... you'll see how convoluted it is, even for this simple endpoint, though perhaps this is the most important one to test due to the lack of pydantic models.

CasperWA

Just adding all investigated counts for all tests.
And great work on the test!!

tests/validator/test_utils.py

- Make checks on type and header parameter in content-type for '/versions' optional additionally allowing 'text/plain' (closes #669) - Report correct base URL on /versions validation failure (closes #668) - Added mock tests for /versions in various scenarios

ml-evs · 2021-01-15T17:32:19Z

Resolved and rebased back into one commit, should be good to go! Probably worth a release after this too.

CasperWA

Merge away if all checks succeed 👍

ml-evs requested a review from CasperWA as a code owner January 11, 2021 15:10

ml-evs marked this pull request as draft January 11, 2021 15:22

ml-evs force-pushed the ml-evs/validator_fixes branch from de34aad to 2cccd7a Compare January 11, 2021 15:27

ml-evs marked this pull request as ready for review January 11, 2021 15:32

CasperWA previously approved these changes Jan 11, 2021

View reviewed changes

optimade/validator/validator.py Outdated Show resolved Hide resolved

optimade/validator/validator.py Outdated Show resolved Hide resolved

ml-evs dismissed CasperWA’s stale review via 162f957 January 11, 2021 16:59

ml-evs force-pushed the ml-evs/validator_fixes branch from 162f957 to 25cf7bf Compare January 11, 2021 17:00

ml-evs requested a review from CasperWA January 11, 2021 17:00

CasperWA previously approved these changes Jan 11, 2021

View reviewed changes

ml-evs marked this pull request as draft January 11, 2021 17:04

ml-evs mentioned this pull request Jan 11, 2021

/versions header Content-Type value should be granularized according to RFC requirements in validator #669

Closed

ml-evs dismissed CasperWA’s stale review via acbb026 January 11, 2021 17:24

ml-evs force-pushed the ml-evs/validator_fixes branch from acbb026 to ed09680 Compare January 11, 2021 17:27

ml-evs force-pushed the ml-evs/validator_fixes branch from ed09680 to 1987f3f Compare January 11, 2021 19:49

ml-evs marked this pull request as ready for review January 11, 2021 20:09

ml-evs requested a review from CasperWA January 11, 2021 20:09

ml-evs changed the title ~~Make header checks on '/versions` optional~~ Make 'header=present' parameter check on '/versions` optional Jan 11, 2021

ml-evs force-pushed the ml-evs/validator_fixes branch from 1987f3f to 4f6d15b Compare January 12, 2021 14:54

ml-evs changed the title ~~Make 'header=present' parameter check on '/versions` optional~~ Make content-type response checks on '/versions` endpoint optional Jan 12, 2021

ml-evs force-pushed the ml-evs/validator_fixes branch from c9af7d0 to 58502b1 Compare January 15, 2021 14:25

ml-evs force-pushed the ml-evs/validator_fixes branch from 58502b1 to d9b48e6 Compare January 15, 2021 15:27

CasperWA requested changes Jan 15, 2021

View reviewed changes

optimade/validator/validator.py Outdated Show resolved Hide resolved

optimade/validator/validator.py Outdated Show resolved Hide resolved

CasperWA requested changes Jan 15, 2021

View reviewed changes

tests/validator/test_utils.py Show resolved Hide resolved

tests/validator/test_utils.py Show resolved Hide resolved

tests/validator/test_utils.py Show resolved Hide resolved

tests/validator/test_utils.py Show resolved Hide resolved

tests/validator/test_utils.py Show resolved Hide resolved

ml-evs force-pushed the ml-evs/validator_fixes branch from 3e89cd8 to fe9a910 Compare January 15, 2021 17:31

ml-evs requested a review from CasperWA January 15, 2021 17:31

CasperWA approved these changes Jan 15, 2021

View reviewed changes

ml-evs merged commit 9569362 into master Jan 15, 2021

ml-evs deleted the ml-evs/validator_fixes branch January 15, 2021 17:39

ml-evs added validator Related to the OPTIMADE validator bug Something isn't working enhancement New feature or request labels Jan 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make content-type response checks on '/versions` endpoint optional #670

Make content-type response checks on '/versions` endpoint optional #670

ml-evs commented Jan 11, 2021 •

edited

codecov bot commented Jan 11, 2021 •

edited

ml-evs commented Jan 11, 2021

CasperWA left a comment

ml-evs commented Jan 11, 2021

CasperWA commented Jan 11, 2021 •

edited

CasperWA commented Jan 11, 2021

ml-evs commented Jan 11, 2021

ml-evs commented Jan 11, 2021

ml-evs commented Jan 15, 2021

CasperWA left a comment •

edited

ml-evs commented Jan 15, 2021

CasperWA left a comment •

edited

ml-evs commented Jan 15, 2021

CasperWA left a comment

Make content-type response checks on '/versions` endpoint optional #670

Make content-type response checks on '/versions` endpoint optional #670

Conversation

ml-evs commented Jan 11, 2021 • edited

codecov bot commented Jan 11, 2021 • edited

Codecov Report

ml-evs commented Jan 11, 2021

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Jan 11, 2021

CasperWA commented Jan 11, 2021 • edited

CasperWA commented Jan 11, 2021

ml-evs commented Jan 11, 2021

ml-evs commented Jan 11, 2021

ml-evs commented Jan 15, 2021

CasperWA left a comment • edited

Choose a reason for hiding this comment

ml-evs commented Jan 15, 2021

CasperWA left a comment • edited

Choose a reason for hiding this comment

ml-evs commented Jan 15, 2021

CasperWA left a comment

Choose a reason for hiding this comment

ml-evs commented Jan 11, 2021 •

edited

codecov bot commented Jan 11, 2021 •

edited

CasperWA commented Jan 11, 2021 •

edited

CasperWA left a comment •

edited

CasperWA left a comment •

edited