Hi Ludwig, On Fri, May 28, 2021 at 4:10 PM Ludwig Nussel wrote:

Hi,

tl;dr if you rely on package reviews for your development process in OBS make sure requests have a revision. Only osc shows the information you are looking for.

Turns out submit request in OBS do not necessarily refer to a specific revision in the originating package. Means the sources can change after a review passed. Unfortunately changing the sources does not reset reviews and reviews do not record the revision they were done for. That obviously defeats the purpose of source reviews. Accepting a request without revision may therefore lead to accepting something different from what was reviewed (resp staged).

Thanks for publicizing this issue, would it be possible to get a patch to disable this functionality? Doing this per-project basis sounds fine but for some OBS instances the alternative is never desired. Regards, ismail

On Wed, Jun 2, 2021 at 7:39 AM Adrian Schröter wrote:

On Dienstag, 1. Juni 2021, 20:05:43 CEST İsmail Dönmez wrote:

...

Hi Ludwig,

On Fri, May 28, 2021 at 4:10 PM Ludwig Nussel wrote:

...

Hi,

tl;dr if you rely on package reviews for your development process in OBS make sure requests have a revision. Only osc shows the information you are looking for.

Turns out submit request in OBS do not necessarily refer to a specific revision in the originating package. Means the sources can change after a review passed. Unfortunately changing the sources does not reset reviews and reviews do not record the revision they were done for. That obviously defeats the purpose of source reviews. Accepting a request without revision may therefore lead to accepting something different from what was reviewed (resp staged).

Thanks for publicizing this issue, would it be possible to get a patch to disable this functionality? Doing this per-project basis sounds fine but for some OBS instances the alternative is never desired.

it also breaks functionality like submitting modified link information.

One could enforce it always via an config/options.yml setting, but it has more consequences therefore.

It was always communicated that it is up to the reviewer to decide if he wants to accept this or not (people should be used to this situation as it is also the default on platforms like github). It is nothing new since more then 10 years after all...

This sounds all good and fine but: 1. WebUI does not show this at all, not everyone reviews via osc. Especially outside SUSE. 2. Any reviews done should be obsoleted when a new revision arrives (ala GitHub) but this doesn't happen. Regards, ismail

Hi, On 2021-05-28 16:10:25 +0200, Ludwig Nussel wrote:

tl;dr if you rely on package reviews for your development process in OBS make sure requests have a revision. Only osc shows the information you are looking for.

Note that just relying on the presence of a revision is not "sufficient" because it can point to a link (see below). <SNIP>

AFAIK this feature has been in OBS since the very beginning, it's just well hidden as the official interfaces osc and the webui do add a revision when creating requests. There is no way to turn that off either. Also tools such as bots that use osc as python module automatically generate submit request with revision. This is not because the server enforces it but because the client code does it.

In case of the osc lib, it depends on how you use it. For instance, if you create a request via r = osc.core.Request() r.add_action('submit', src_project='openSUSE:Tools', src_package='osc', tgt_project='home:Marcus_H', tgt_package='abc') r.create(conf.config['apiurl']) no revision is added. However, if you use r.create(conf.config['apiurl'], addrevision=True) the API takes care of adding a revision. <SNIP>

Meanwhile the factory-auto bot was enhanced to decline unversioned requests to Factory (thanks Fabian).

Does it also check if the specified revision points to an expanded file set? For instance, let's assume that - prj/tgt is a plain package (no _link file) - prj/lnk is a link to prj/tgt Now, create a request via r = osc.core.Request() rev = '40e1a6ff74681c68a001adc3ca0c6474' r.add_action('submit', src_project='prj', src_package='lnk', src_rev=rev, tgt_project='prj', tgt_package='bar') r.create(conf.config['apiurl']) where 40e1a6ff74681c68a001adc3ca0c6474 points to an unexpanded file set (that is, it has a _link file). Such a request is displayed like this $> osc rq show 1234 Request: #1234 submit: prj/lnk@40e1a6ff74681c68a001adc3ca0c6474 -> prj/bar ... $> Even if you now run "osc rq show -d 1234", the "expanded" diff is displayed. That is, it is probably not apparent from a reviewer's POV that rev 40e1a6ff74681c68a001adc3ca0c6474 is in fact a link. Now, if a review is accepted, the "attacker" can modify prj/lnk's link target. If the request is eventually accepted, the modified files end up prj/bar. Marcus