search

Yelp / swagger_spec_validator

Other
104 stars 71 forks source link

Pin jsonschema version to prevent backwards-incompatibility #118

Closed hadizh closed 5 years ago

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 98.526% when pulling 84995afd9b8b80070f6e12c8c70df4db680fe4bb on hadizh:master into 1d9b43de9f6770eb1a1c00eab831945a5a4ab561 on Yelp:master.

macisamuele commented 5 years ago

Hey @hadizh thanks a lot for contributing to the project. I would like to ask you some more details related to this change considering that swagger-spec-validator>=2.4.0 should be able to properly operate with jsonschema-validator==3.0.0.

joetempleman commented 5 years ago

We just ran into this issue because this wasn't pinned so I have a small amount of context.

There is an older version of bravado-core that referenced private members inside of jsonschema (fixed in this PR https://github.com/Yelp/bravado-core/pull/304/files).

That private member (jsonschema._validators.type_draft4) has since been removed and since both dependencies are unpinned you can get a situation where you have the older bravado-core and the newer jsonschema which are incompatible.

For the sake of other developers depending on this package please pin your dependencies so that we don't have random failures based on the package cache of the node it happens to be being installed on.

sjaensch commented 5 years ago

If you're developing an application (a piece of code that is going to be run as-is, not a library) then you should pin all of your dependencies, both direct and indirect. This is just standard best practice and something we generally do pretty well internally.

There's always going to be incompatibilities that cannot be properly expressed in setup.py. The case you describe is a good example: we can't modify already-released bravado-core versions to add a restriction on which jsonschema version is supported, and jsonschema can't put restrictions on its upstream users.

The major version change to jsonschema implies potentially breaking changes. If you do not pin your dependencies then you will invariably run into issues like these. This is a concious choice you make - you get the newest everything, but things might randomly break. It's not something we can fix at the library level.

joetempleman commented 5 years ago

I totally agree on pinning versions of dependencies in our code but don't you think that knowing that your code doesn't work with jsonschema>3.0 that you should express that in your dependencies?

sjaensch commented 5 years ago

@joetempleman absolutely, completely agree there. But the current version of swagger-spec-validator does work with jsonschema 3. So what do you propose we should do?