Code review comment for ~pfsmorigo/ubuntu-cve-tracker:pfsmorigo/reason_in_notes

On Thu, Aug 10, 2023 at 08:35:58PM -0000, Emilia Torino wrote:
> Leaving one question just in case, otherwise LGTM!
>
> Diff comments:
>
> > diff --git a/scripts/publish-cves-to-website-api.py b/scripts/publish-cves-to-website-api.py
> > index d6af0e7..d5a0042 100755
> > --- a/scripts/publish-cves-to-website-api.py
> > +++ b/scripts/publish-cves-to-website-api.py
> > @@ -163,6 +163,10 @@ def post_single_cve(cve_filename):
> >
> > notes = []
> >
> > + # TODO Remove this when we have the proper field ir ready
>
> s/ir/is? Seems a typo :), also if thats the case maybe "we have" should be removed? So either remove everything after field, or remove "we have" I guess
>
> > + if cve_data["Priority"][1]:
>
> Should we check cve_data["Priority"] length before accessing the reason at index 1?

In hindsight, it would have been useful to have hidden the accessors behind an
api in cve_lib, something like

  cve_lib.cve_get_priority(cve)
  cve_lib.cve_get_priority_reason(cve)

so that we can move to a future where the internal structures of cves
are hidden in a class. That said, I don't know how to make the api
export the case where we have a different priority for a specific
package or release.

> > + notes.append({"author": "", "note": "Priority reason:\n" + cve_data["Priority"][1]})
> > +
> > for [author, note] in cve_data["Notes"]:
> > notes.append({"author": author, "note": note})
> >
>
>
> --
> https://code.launchpad.net/~pfsmorigo/ubuntu-cve-tracker/+git/ubuntu-cve-tracker/+merge/448821
> You are subscribed to branch ubuntu-cve-tracker:master.
>

--
Steve Beattie
<email address hidden>