The default policy is missing actions for the sub policy lines.

I started to say that the default policy needs further adjustment too, but after looking at it, I'm not sure how easily we can accomplish backwards compatibility this way.

• policies used with {assert,check}_policy expect results of the form "(allow|deny) <reason>" and warn if the first word is anything else
• policies used with PolicyTest only check if result.lower() in ('yes', 'true', 'allow') and do not handle reason
• I don't see a simple, clean way to propagate the reason out of the subpolicy

Kinda wonder if the policy should be named more specifically since it is only called for rpm builds (which, granted, have 'build' as the task method name)

Before I realized you were referencing the old policies in the new default, I was thinking that we should maintain the old codepath (deprecated) for a bit to maintain compat. That is still an option, though I don't think there's a good way in kojid to check which policies are defined. Still, something like this is another way we could go.

The changes I see since last review are to rename the new policy to build_rpm and to add actions to subpolicy check lines in said policy.

This still doesn't address the backwards compatibility issue that stems from the fact that invoking a policy as a subpolicy (via the policy test) has different behavior than invoking a policy via {assert,check}_policy. In particular, any use of reason args in the actions of the subpolicy will not work correctly. If a policy includes an allow with a reason, it will be treated as false result.

Hmm, finally got it :-) It would mean adding new policy aka "check_policy". Is it worth it or should we just leave those old policies for now?

I think the shortest path to a working, backwards compatible implementation is to preserve the old policy calls, but recommend that folks use the build policy instead of the older ones.

We might eventually want deprecate the old policies and tell people they have to migrate

I've moved new policy call below the original ones and reset their default values to 'allow', so if policy is defined, it can forbid the execution before build_rpm, otherwise default rules of build_rpm behave same as original rules before. So, it should be backwards-compatible now.

This looks much more straightforward, but the default build_rpm policy as defined will deny all non-scratch builds for non-admins.

Ok, I've changed it to allow :: all

Current changes to default policies remove past checks that block non-scratch srpm builds and repo_id builds for non-admins. We need to either:

1. keep those checks in the default build_from_{srpm,repo_id} policies and leave build_rpm as allow :: all
2. migrate those checks to the new build_rpm policy, leaving the old policies defaulting to allow all

If we want to put the checks on build_rpm, I think we'd need the following:

'build_rpm' : '''
    bool from_srpm :: {
        bool scratch !! {
            has_perm admin !! deny Only admin can do this via default policy
            }
        }
    bool repo_id :: {
        has_perm admin !! deny Only admin can do this via default policy
        }
    '''

Unfortunately, the limits of policy syntax and the parity of our flags to check make the check a little obtuse.

Leaving the checks where the were for now would be the most compatible, but of course we know we want to phase those policies out. Still, could be sensible to start slow.

I'm for the first option - otherwise we will have to make another announcement that default policies are changed.

Btw, I've changed line bool from_srpm :: { to bool from_scm !! {

I'm for the first option - otherwise we will have to make another announcement that default policies are changed.

Indeed. And we'd also need a detailed migration note for people that are currently using non-default policies (if we go with option 2).

Btw, I've changed line bool from_srpm :: { to bool from_scm !! {

Good catch, but I'm confused. I thought you wanted to go with option 1.

While we'll eventually need force people to migrate, it might be nice to have a release where the build_rpm policy is an optional new feature rather than an incompatible change.

Description of the old policies still says "default is now allow to not block build_rpm policy".

Other than that, lgtm

Commit 1b8a73e6 fixes this pull-request