Comment 7 for bug 1988366

Revision history for this message
Robie Basak (racb) wrote :

SRU review

> +-version_attributes = set(["lio_version", "version"])
> +-discovery_auth_attributes = set(["discovery_auth"])

These might be accessed by an API caller somewhere, and so would represent a regression. I spent some time digging around and didn't find a direct example, so I was going to leave it, but then I came across in rtslib/tcm.py:

> from .fabric import target_names_excludes

target_names_excludes is being maintained so I think this is OK. However, I think it demonstrates that "stuff" does make use of these names arbitrarily and so we probably shouldn't be dropping items from the module namespace if we don't have to. Would there be a problem with just dropping the dropping of these two lines, to reduce the risk that we'll break something somewhere, including something external to the archive that we cannot find, by unnecessarily dropping these? It should be trivial to do and could save users quite a bit of pain if they are impacted, even if unlikely.

Or, put another way, if we were writing this directly for Jammy, we certainly wouldn't be doing the refactoring that is being done here because it is riskier, so it seems to me that we shouldn't, and the change I suggest should be trivially safe. So I think we should do that.

Apart from this, +1 from an SRU perspective.

If you disagree let's discuss, so I'll leave this in the queue for now. If you agree, please upload an adjustment.