Adds docs for plugin writers on declaring deps #3176
Conversation
|
@pulp/core I'm looking for two reviews for this docs PR. This is designed to be a general policy, so not just language and clarity, but on the recommendations too. Thanks! |
| ``jsonschema<5.0`` - This is appropriate if ``jsonschema`` could release breaking changes in | ||
| ``jsonschema`` 5.0 and you are compatible with 4.* and lower. | ||
|
|
||
| ``jsonschema~=4.4`` - This would be appropriate if you have a known incompatibility with |
There was a problem hiding this comment.
I think we should a void these, because dependabot will try to upgrade this to jsonschema~=5.0 once that is released. Instead jsonschema>=4.4,<5.0 will be upgraded to jsonschema>=4.4,<6.0 eventually, as far as i understand.
The greatest problem i see is that we never test that our lover bounds are (still) correct.
There was a problem hiding this comment.
To make sure I understand, is the idea that I show the ~= example and then explain it should be avoided?
There was a problem hiding this comment.
The first question is: Do we agree it should be avoided. If yes, then your question.
| ``jsonschema<4.4`` *and* it's possible ``jsonschema`` could release breaking changes in | ||
| ``jsonschema>=5.0``. | ||
|
|
||
| ``jsonschema==4.4.0`` - This is a last resort and needs an exceptional reason to do so. |
There was a problem hiding this comment.
can we say that such declaration makes it very high probability it will create version conflicts with other plugins?i mean this is self explanatory but still..
There was a problem hiding this comment.
I think calling it out as discouraged is my intention with the PR. I didn't write that, but after reading what I wrote that is what makes sense to me. Basically these types of dependency declarations is what this PR is all about.
There was a problem hiding this comment.
@ipanova would you be ok w/ me pushing a revision like that?
|
can you also add a paragraph about transitive vs explicit deps declaration - for example, python-gnupg might be currently pulled in by pulpcore but te plugin should explicitly define its dependency on python-gnupg |
I'm not sure about this. At least we need some good guidance around this We currently have pulpcore and pulp_ansible pinning two different versions of aiofiles. I'd rather consider the availablility of aiofiles as being part of the plugin-api. But maybe we need to state the libraries that are. And at that point adding or removing one is a breaking change. |
bmbouter
force-pushed
the
add-version-range-docs
branch
2 times, most recently
from
d65dd1a
to
8a77e83
Compare
| uses Django directly, your plugin should declare its dependency on Django. | ||
|
|
||
| Why? To continue with the Django example... Django can introduce breaking changes with each | ||
| release, so if your plugin relies on pulpcore to declare the Django requirement, and then |
There was a problem hiding this comment.
The only problem i have is with this specific example you decided to take - Django is being used in pulpcore and plugin api. We have our policy and promise of not breaking our plugins with the plugin-api policy we have in place, coming back to what @mdellweg said in this comment #3176 (comment) - dependencies that are part of plugin api, in my opinion should not be explicitly declared in plugins. Plugin writers have the whole right to rely on our breaking changes policy.
There was a problem hiding this comment.
Django isn't directly part of the plugin API though. Things get so much more complicated when we start to think that pulpcore is "delivering" the dependency tree. What motivation do we have for that?
Here's an example showing why I think that approach would be intractable. Say pulpcore uses library 'foo' and so does some plugin. foo comes out with a new version which could have breaking changes in it, but they don't affect pulpcore and the pulpcore CI says everything is fine so we increase the upper bound. Oh wait though, we've just broken the plugin which was affected by the breaking change in 'foo' yet that plugin didn't run as part of the CI testing the new version of 'foo'.
Now imagine ^ same story, only with 'bar' which is a dependency of 'foo', and some plugin happens to also use 'bar'. What I see in this is that no dep upgrade could ever be safely evaluated all the way down the dependency tree.
There was a problem hiding this comment.
My main takeaway is that bumping the lower bound of a dependency is a breaking change, and new features that need such a change (we will probably never recognize this, making matters worse) can only be introduced in a breaking change release. The same goes for adding a new dependency. (Removing one is now absolutely fine.)
There was a problem hiding this comment.
Yes it's a breaking change, but semver allows breaking changes with bugfixes.
What can I do to improve (or merge) this PR?
There was a problem hiding this comment.
I feel that the term "as broad a range [...] as possible" is dangerous. Can you add that extra care and awareness is needed to keep the lower bound of a dependency in sync with the features used from that library? And also that bumping the lower bound may break plugin compatibilities? We should at least recognize that there is danger.
BTW: Even when not importing from a library directly, a plugin may be faced to interact with its objects. So pulpcore adopting a new version can break plugins in any case. In the end we need to accept that the pulp plugin api is not an island.
There was a problem hiding this comment.
I keep wondering, if this is the working and accepted way to do things in the python ecosystem, why do we even need to write a single line about it?
There was a problem hiding this comment.
I keep wondering, if this is the working and accepted way to do things in the python ecosystem, why do we even need to write a single line about it?
It's the norm for Python software that operates as a "library" because those softwares are installed side-by-side a lot of other software it can't control. End-user python code though tends to be the only thing installed in an environment so there is really only one dependency tree. I believe we need clarification on this because Pulp had been operating as if it was the latter but really it's the former.
There was a problem hiding this comment.
@mdellweg ty for the comment it outlines pretty clearly the two things I should add:
- dependabot should handle the upper bound and plugin writers are enocuraged to use it
- reasons for increasing the lower bound, e.g. usage of new features or bugfixes only available in newer versions
Let me add those and you can tell me what you think.
There was a problem hiding this comment.
While working on the revisions I found this, which aligns I think what we're recommending pretty well too https://pip.pypa.io/en/latest/topics/dependency-resolution/#loosen-your-top-level-requirements
There was a problem hiding this comment.
The big takeaway for me is: "Try to have less dependencies." aka: "Ask yourself if it is necessary to add another dependency!" Can we add something like that too?
That is ok as long as it's not done in a bugfix release. |
bmbouter
force-pushed
the
add-version-range-docs
branch
from
8a77e83
to
5a139aa
Compare
|
I'm declining to add a part about having fewer dependencies since I believe if there's a dependency we should use it. This has downsides though so I also won't add a statement suggesting that either. Thank you all for the engaging discussion and reviews! |
closes #2997