[Reportbug-maint] Bug#878088: Bug#878088: reportbug: please inform security and lts teams about security update regressions

[Guido Günther]

Hi,
On Mon, Jan 29, 2018 at 01:29:59PM +0100, Markus Koschany wrote:
> [sorry forgot to CC the rest]
> 
> Hello,
> 
> Am 29.01.2018 um 09:30 schrieb Guido Günther:
> > Hi Markus,
> > thanks for pursuing this! Some minor nitpicks:
> >
> > On Mon, Jan 29, 2018 at 12:11:03AM +0100, Markus Koschany wrote:
> > [..snip..]
> >> +            if utils.is_security_update(package, pkgversion):
> >> +                if ui.yes_no('Do you want to report a regression
> because of a security update? ',
> >> +                             'Yes, please inform the LTS and
> security teams.',
> >> +                             'No or I am not sure.', True):
> >> +                    regex = re.compile('(\+|~)deb(\d+)u(\d+)')
> >> +                    secversion = regex.search(pkgversion)
> >> +                    distnumber = secversion.group(2)
> >> +                    support = 'none'
> >
> > Using
> >
> >    support = None
> >
> > is a bit more pythonic.
> 
> support = None was part of the very first iterations of this patch but
> the string 'none' is used in distributions.json nowadays. I initialize
> the variable with the same value and then I compare the value in our
> support key. None is not equal to 'none' and caused an error when
> Salvatore changed this value in distributions.json.
> 
> >> +                    email_address = []
> >> +                    try:
> >> +                        r =
> requests.get('https://security-tracker.debian.org/tracker/distributions.json',
> >> +                                    timeout=self.options.timeout)
> >
> > This will not catch 404 or similar http status codes. If you don't care
> > about the type of error you can just do …
> >
> >                            r.raise_for_status()
> >
> > … ff you dont handle the error this …
> >
> >> +                        data = r.json()
> >
> > … will fail like:
> >
> [...]
> >
> > … The raise_for_status exception is caught here alreadyx since
> > requests.exceptions.HTTPError is a subclass of
> > requests.exceptions.RequestException):
> [...]
> 
> I thought requests.exceptions.RequestException is the exception base
> class of Python requests and catches all cases? If not, would

Yes, that catch is already correct (just wanted to point this out). The
r.raise_for_status() is the important part to not trip up r.json().
Cheers,
 -- Guido

> 
> except (requests.exceptions.RequestException,
> requests.exceptions.HTTPError):
> 
> suffice?
> 
> > It would also improve readability if this whole code block moved into
> a get_security_contact()
> > function.
> 
> Maybe.
> 
> Markus
> 
> 
> 
>