Add depency resolving on advanced copy endpoint - #1459
Conversation
17d1f84 to
cf61187
Compare
|
Hi @quba42 , this PR should be ready for review. I am not sure about the failing check but it doesn't look related. Can you (or someone else) take a look? |
|
Hi @quba42, did you have the time to take a look? Or maybe you know someone who could review this PR. |
cf61187 to
a470c23
Compare
a470c23 to
8ac3065
Compare
|
First of all my apologies for taking so long to react. This is partly because of a lack of time, but also partly because I was and remain unsure how to respond to this submission in particular. First for some context: We obviously created the interface (to match pulp_rpm) and then placed a not implemented error behind it, which was probably a bad idea in terms of expectations management. At the time we had pretty concrete plans and attempts to implement dependency solving. Our plan was to implement a custom DB type for debversion first, for the sake of efficient version comparisons in the DB. At the time we were unable to overcome certain technical hurdles in that approach and the entire project disappeared into our backlog. Note: There is actually another attempt at adding the debversion type open now: #1335, which is also languishing because we have not reviewed it. In the mean time, the conversation I see amongst pulp_rpm maintainers has turned into: "dependency solving is a mess of corner cases all the way down, and while some people sometimes need it, we really want to discourage its use. Ideally we would like to drop this feature, but we probably can't." As a result I pretty much stopped thinking about adding this feature all together. I am sure there is still a need and desire for this feature (as evidenced by this submission), but I also don't see the capacity within our team to properly review or maintain it. IMHO a good review for this submission requires some real work. Starting with re-familiarizing oneself with the dependency (and version level dependency) specification and then checking if the current implementation and test coverage being added covers all known edge cases. Performance for large repositories with correspondingly large dependency trees is also a concern. This in turn requires large manual integration tests. One option to prevent this from becoming a endless review process, might be to accept a less rigorous approach, and add the changes as a tech preview. In fact, we probably want to do that either way, just to give ourselves some wiggle room if there are still issues with whatever initial implementation is ultimately merged. Adding some docs to this effect and perhaps logging a warning when dependency solving is used would be enough to declare this as a tech preview. Having explained all of this background where I am currently coming from, I would like to ask some general high level questions before taking things further. Given you went through the trouble of implementing this submission I am sure you have some thoughts:
The good news is that this feature is neatly tucked away behind its feature flag. We can be certain that adding it does not affect anyone not actively choosing to use it. |
closes #386