ppa-dev-tools

Code review comment for ppa-dev-tools:add_rdepends_argument

Revision history for this message

Bryce Harrington (bryce) wrote on 2023-04-25:

> Thanks for the detailed explanation, Bryce.
>
> Yes, absolutely no problem to leave this further automation for the next
> release. I'm glad that we're on the same page on this topic!
>
> I'm leaving a few cosmetic comments below, but otherwise the code looks good
> to me, so I'm approving the MP. But I have a few other general comments.
>
> - I set up a container in order to fully test the package (including
> installing it), and I stumbled upon a few problems with missing requirements
> from the requirements-dev.txt file.

If you can shoot me either the steps you followed, or the changed requirements, it'd be appreciated as I prepare the release. I posted an MP the other day with some packaging work, that includes adding a missing dependency or two, but could be more. I also found that running tox on the codebase scared up some requirements issues that I will try to look at if not for this release than for a future one. Getting tox to pass without issue would be very nice, and probably would take care of the issues you hit here.

> - When I tried running the "tests --show-rdepends" command against a public
> PPA, it asked me to log into LP. This shouldn't really be necessary, unless
> dealing with private PPAs and such. It's not something really important to
> address right now (I understand that you have other things on your plate), but
> it's something to keep in mind IMHO.

Noted, I'll put this on the list to work on later.

I know exactly what the problem is - when constructing the Launchpad service you specify what level of permissions you need, and I just across the board ask for read/write privs regardless of what the individual command actually needs. Fixing it such that commands only request the priv level they need will take a bit of plumbing, however as an outgrowth of some of the experimentation I've done with qastaging I'm realizing I need a more sophisticated system for selecting Launchpad service API versions, permissions, and so on.

> Thanks again for working on this.

> Thanks for the detailed explanation, Bryce.
> 
> Yes, absolutely no problem to leave this further automation for the next
> release.  I'm glad that we're on the same page on this topic!
> 
> I'm leaving a few cosmetic comments below, but otherwise the code looks good
> to me, so I'm approving the MP.  But I have a few other general comments.
> 
> - I set up a container in order to fully test the package (including
> installing it), and I stumbled upon a few problems with missing requirements
> from the requirements-dev.txt file.

If you can shoot me either the steps you followed, or the changed requirements, it'd be appreciated as I prepare the release.  I posted an MP the other day with some packaging work, that includes adding a missing dependency or two, but could be more.  I also found that running tox on the codebase scared up some requirements issues that I will try to look at if not for this release than for a future one.  Getting tox to pass without issue would be very nice, and probably would take care of the issues you hit here.

> - When I tried running the "tests --show-rdepends" command against a public
> PPA, it asked me to log into LP.  This shouldn't really be necessary, unless
> dealing with private PPAs and such.  It's not something really important to
> address right now (I understand that you have other things on your plate), but
> it's something to keep in mind IMHO.

Noted, I'll put this on the list to work on later.

I know exactly what the problem is - when constructing the Launchpad service you specify what level of permissions you need, and I just across the board ask for read/write privs regardless of what the individual command actually needs.  Fixing it such that commands only request the priv level they need will take a bit of plumbing, however as an outgrowth of some of the experimentation I've done with qastaging I'm realizing I need a more sophisticated system for selecting Launchpad service API versions, permissions, and so on.

> Thanks again for working on this.

« Back to merge proposal