Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

openapicore case insensitive header fix #88

Closed

Conversation

sjiekak
Copy link
Contributor

@sjiekak sjiekak commented Jul 27, 2020

fixes #87

@zupo
Copy link
Collaborator

zupo commented Jul 27, 2020

Fix looks good but my "I'll fix it later" CI script that enables us to test the minimal supported version of openapi-core needs to be updated any time we bump openapi-core: https://github.com/Pylons/pyramid_openapi3/blob/master/inject_minimal_openapi-core.sh

I likely won't be able to do this before next week, so if you are in a hurry to get this PR merged, you can try and see if you can decipher how it works.

@sjiekak
Copy link
Contributor Author

sjiekak commented Jul 28, 2020

So I'm bumping the script as well. I preserved the the hash order in the Pipfile.lock file as well as how the minimal supported version was injected, even if they are currently the same.

grep "==0.13.3" Pipfile.lock
sed -i 's/"==0.13.3"/"==0.13.1"/g' Pipfile.lock
grep "==0.13.4" Pipfile.lock
sed -i 's/"==0.13.4"/"==0.13.4"/g' Pipfile.lock
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sed -i 's/"==0.13.4"/"==0.13.4"/g' Pipfile.lock
sed -i 's/"==0.13.4"/"==0.13.1"/g' Pipfile.lock

grep "57973b7383214a529012cf65ddac8c22b25a4df497366e588e88c627581c2928" Pipfile.lock
sed -i s/57973b7383214a529012cf65ddac8c22b25a4df497366e588e88c627581c2928/d61305484f8fefda78fdddaf8890af6804fae99dff94013abd9480873880bddc/g Pipfile.lock
grep "b8b4283d84038495fb3bde4a868cd9377272ecf898f142d7706d6d49fc14d218" Pipfile.lock
sed -i s/b8b4283d84038495fb3bde4a868cd9377272ecf898f142d7706d6d49fc14d218/b8b4283d84038495fb3bde4a868cd9377272ecf898f142d7706d6d49fc14d218/g Pipfile.lock
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sed -i s/b8b4283d84038495fb3bde4a868cd9377272ecf898f142d7706d6d49fc14d218/b8b4283d84038495fb3bde4a868cd9377272ecf898f142d7706d6d49fc14d218/g Pipfile.lock
sed -i s/b8b4283d84038495fb3bde4a868cd9377272ecf898f142d7706d6d49fc14d218/d61305484f8fefda78fdddaf8890af6804fae99dff94013abd9480873880bddc/g Pipfile.lock

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.13.4's hash needs to be replaced by our "minimal supported version of openapi-core"'s hash, so that this minimal version gets tested on CI.

@sjiekak
Copy link
Contributor Author

sjiekak commented Jul 28, 2020

Hey @zupo. Unless we write retro-compatible code, this version will not be able to support older version of openapi-core

@sjiekak
Copy link
Contributor Author

sjiekak commented Jul 28, 2020

Something like this

        header = request.headers.items()
        if openapi_core.__version__ < '0.13.4':
            header = request.headers

@zupo
Copy link
Collaborator

zupo commented Jul 28, 2020

Ah, I understand it now, sorry for the noise.

I don't have a strong opinion about whether we need to write retro-compatible code, I usually stick to latest releases. But if you want you can add retro-support, I'm really fine either way.

Can you make sure that we update the minimal version to 0.13.4 wherever 0.13.1 is mentioned? Probably some constraints somewhere? (sorry for vagueness, not at may workstation)

@sjiekak
Copy link
Contributor Author

sjiekak commented Jul 28, 2020

Can you make sure that we update the minimal version to 0.13.4 wherever 0.13.1 is mentioned

Thanks for the remark, I forgot to update the setup file.

Copy link
Collaborator

@zupo zupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@zupo
Copy link
Collaborator

zupo commented Jul 28, 2020

Ah, snap. The way CI is set up is that it shares pipenv caches between py37 and py38 if Pipfile.lock is not different between the two -> which with this PR, Pipfile.lock is the same, since minimal openapi-core version is the same as current openapi-core version. And sharing the cache makes pipenv break.

I'll think about it. Probably add retro-compatiblity becaues it's easier then fiddleing with CI.

@sjiekak sjiekak force-pushed the openapi-0.13.4-request-parameters branch from 6d705f4 to 8b78630 Compare August 3, 2020 17:58
@sjiekak sjiekak requested a review from zupo August 3, 2020 18:15
@sjiekak
Copy link
Contributor Author

sjiekak commented Aug 3, 2020

I wrote a retro-compatible version. I feel like there is an action needed on your part to clean up the cache.

@zupo
Copy link
Collaborator

zupo commented Aug 16, 2020

Superseded by #89.

@zupo zupo closed this Aug 16, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pyramid_openapi3 incompatible with openapi-core 0.13.4
2 participants