kojid.bak probably shouldn't be part of PR.

It is problematic in case of additional types (1|2 == 1|2|3 == 3)

I can't apply this and I'm not completely sure if it is ok.
1) Does it make sense to have draft nullable? Why it is not the same as in build table?
2) If I've rpm from internal build it has NULL after this migration but also external_repo_id 0 which fails the check.

draft=draft?

It is never used with draft=True in kojid.

promoteBuild would fail in applyVolumePolicy due to missing data in binfo. It could be solved by reusing whole binfo with updated release:

@@ -13755,7 +13755,8 @@ class RootExports(object):
assert_policy('draft_promotion', policy_data, force=force)
# volume check, deny it if volume is changed as it's only allowed for admin
# after building, see applyVolumePolicy
-        new_volume = apply_volume_policy(target_build, strict=False, dry_run=True)
+        binfo['release'] = target_release
+        new_volume = apply_volume_policy(binfo, strict=False, dry_run=True)
if new_volume is not None and new_volume['id'] != binfo['volume_id']:
# probably we can just apply the volume change here
if strict:

I can't get working code if I don't use something like:

@@ -6366,7 +6366,7 @@ def import_build(srpm, rpms, brmap=None, task_id=None, build_id=None, logs=None,
st_old = binfo['state']
koji.plugin.run_callbacks('preBuildStateChange', attribute='state', old=st_old,
new=st_complete, info=binfo)
-        if draft:
+        if binfo.get('draft'):
build['release'] = koji.DRAFT_RELEASE_FORMAT.format(**build)
for key in ('name', 'version', 'release', 'epoch', 'task_id'):
if build[key] != binfo[key]:
@@ -6389,7 +6389,7 @@ def import_build(srpm, rpms, brmap=None, task_id=None, build_id=None, logs=None,
# now to handle the individual rpms
for relpath in [srpm] + rpms:
fn = "%s/%s" % (uploadpath, relpath)
-        rpminfo = import_rpm(fn, binfo, brmap.get(relpath))
+        rpminfo = import_rpm(fn, binfo, brmap.get(relpath), draft=binfo.get('draft'))
import_rpm_file(fn, binfo, rpminfo)
add_rpm_sig(rpminfo['id'], koji.rip_rpm_sighdr(fn))
if logs:
@@ -14925,7 +14926,7 @@ class HostExports(object):
host.verify()
task = Task(task_id)
task.assertHost(host.id)
-        result = import_build(srpm, rpms, brmap, task_id, build_id, logs=logs, draft=False)
+        result = import_build(srpm, rpms, brmap, task_id, build_id, logs=logs, draft=draft)
build_notification(task_id, build_id)
return result

Not sure if there is some missing commit or another issue.

Why should this be RPM specific? Shouldn't this work for any and all types of builds?

Why should this be RPM specific? Shouldn't this work for any and all types of builds?

It would be nice to have it for all types, but let's start with rpms which are most useful now. Other types introduce additional problems.

API listing in the webUI is broken with error:

xmlrpc.client.Fault: <Fault 1: "<class 'TypeError'>: cannot marshal <flag 'DRAFT_FLAG'> objects">

Same error happens when passing any instance of the koji.DRAFT_FLAG.[DRAFT/REGULAR/ALL] to api calls that expects it, like listBuilds(draft=koji.DRAFT_FLAG.DRAFT)

Any regular non-draft builds are failing with

Fault: <Fault 1: '<class \'psycopg2.errors.NotNullViolation\'>: null value in column "draft" of relation "build" violates not-null constraint

promoteBuild would fail in applyVolumePolicy due to missing data in binfo. It could be solved by reusing whole binfo with updated release:
...

Oh my bad. I changed some parts for unittests but they broke the functionality.

And I missed the parts in import_build and completeBuild.

Updated, and will test on my side as well.

It is problematic in case of additional types (1|2 == 1|2|3 == 3)

I think it should be okay. Even if the input is 1|2|3, it is still 3 (ALL).
And we only validate 1(0b01) and 2(0b10) by & (enum.IntFlag.contains). see https://pagure.io/koji/pull-request/3913#_6__1096

But maybe True/False/None is much easier to understand

I can't apply this and I'm not completely sure if it is ok.
1) Does it make sense to have draft nullable? Why it is not the same as in build table?

I make the draft columns different in build and rpminfo table because:

• A build must be a draft or not, so it can't be NULL
• We can not say a rpm in external repo is a draft or not, so I leave it as NULL
• An internal rpm's draft must not be NULL

2) If I've rpm from internal build it has NULL after this migration but also external_repo_id 0 which fails the check.

So IMHO, this should not happen in theory. But maybe I missed some cases?

2) If I've rpm from internal build it has NULL after this migration but also external_repo_id 0 which fails the check.

So IMHO, this should not happen in theory. But maybe I missed some cases?

No in final state, but migration doesn't set it to TRUE for existing rpms. There probably should be another line:

UPDATE rpminfo SET draft = FALSE;

before enforcing constraint.

@mikem can you also check the PR?

No in final state, but migration doesn't set it to TRUE for existing rpms. There probably should be another line:
UPDATE rpminfo SET draft = FALSE;
before enforcing constraint.

Ah, I got it. Updated.

Thanks!

can you also check the PR?

still looking, please do not merge until I ack

There is a lot here, and I think it likely to take several rounds for us to get this right. Following are some comments in no particular order.

draft query options

The decorator and flag approach to handling draft query options is overkill. Using a decorator simply to convert a single arg seems unnecessarily complex when you could simply have a helper function instead.

The most natural way to handle the draft query option is True/False/None:

• None -- no draft condition (i.e. both)
• False -- no draft builds
• True -- only draft builds

I'm not sure that all the calls we've added draft query options to really need that option, but I guess it doesn't hurt.

promoteBuild

It is generally best to avoid putting the bulk of the handler logic directly in the export, though certainly a lot of older code does this. In this case we likely want to have the option for backend code to trigger promotions independent of the access check, so an underlying promote_build or even _promote_build function would be a good idea.

Also, I don't think we should automatically limit regular users from promoting when this would change the volume. This would only happen if an admin had deliberately made a volume policy rule involving the draft flag, since nothing else about the build should be changing here. If an admin has set this up, then presumably that is what they want.

If anything, we could allow the promotion policy to decide this. I.e. by providing policy data that indicates there is a volume change.

** use of build.extra **

The way we're using build.extra here seems questionable. It's strange for such core functionality to require unpacking extra.

** PathInfo changes **

It's unclear what the to_buildinfo and to_rpminfo functions are for. They appear to be unused, and I would not expect any such changes in this work.

** yesno function **

This appears to be a duplication function definition, and none of the rest of the changes refer to it.

** new tag update type **

I'm not sure if this is actually needed? Promotion should not change the content of a build, just it's release value. All the component files are the same, and even the original path is a preserved with a symlink. I don't think that a repo regen is needed. Am I missing something?

As far as I can tell, these current changes do not address the issue of how to correctly map rpm components for buildroots and archives when nvras are no longer unique. This is arguably the most challenging aspect of the feature.

This need applies even in the code paths you have marked with reject_draft because these builds could still either have draft builds in their buildroots or as an image component.

I see that you've added a build opt to get_rpm, but it doesn't look like it's going to be used in these cases.

Added a restriction to promoteBuild api: only COMPLETE draft build can be promoted now.

There is a lot here, and I think it likely to take several rounds for us to get this right. Following are some comments in no particular order.

draft query options

The decorator and flag approach to handling draft query options is overkill. Using a decorator simply to convert a single arg seems unnecessarily complex when you could simply have a helper function instead.

The most natural way to handle the draft query option is True/False/None:

• None -- no draft condition (i.e. both)
• False -- no draft builds
• True -- only draft builds

That makes sense. I've updated them to draft: Optional[bool] = None

I'm not sure that all the calls we've added draft query options to really need that option, but I guess it doesn't hurt.

promoteBuild

It is generally best to avoid putting the bulk of the handler logic directly in the export, though certainly a lot of older code does this. In this case we likely want to have the option for backend code to trigger promotions independent of the access check, so an underlying promote_build or even _promote_build function would be a good idea.

Updated. Thanks!

Also, I don't think we should automatically limit regular users from promoting when this would change the volume. This would only happen if an admin had deliberately made a volume policy rule involving the draft flag, since nothing else about the build should be changing here. If an admin has set this up, then presumably that is what they want.

Ok. I removed that limit to allow volume change while promoting. Because it only happens with draft_flag / release changes, to keep the code simple, apply_volume_policy is called directly which might now the operation with best performance.

If anything, we could allow the promotion policy to decide this. I.e. by providing policy data that indicates there is a volume change.

** use of build.extra **

The way we're using build.extra here seems questionable. It's strange for such core functionality to require unpacking extra.

I think if we don't introduce new table / fields in DB schema as well, we could make it by reversing the target_release from draft_release. Is that ok?

** PathInfo changes **

It's unclear what the to_buildinfo and to_rpminfo functions are for. They appear to be unused, and I would not expect any such changes in this work.

Ahh, I added those for some draft code in kojid to provide nvra uniqueness check for BuildRoot.

Removed

** yesno function **

This appears to be a duplication function definition, and none of the rest of the changes refer to it.

It's also things I didn't clean. Deleted. Thanks!

** new tag update type **

I'm not sure if this is actually needed? Promotion should not change the content of a build, just it's release value. All the component files are the same, and even the original path is a preserved with a symlink. I don't think that a repo regen is needed. Am I missing something?

It's not needed so far. I added this type to provide a way to track the history of promoting actions.
Additionally, when we provide the full support of draft build in BuildRoot, if we allow multiple draft builds (and their target build) in build-tag, the "only" one in repo should be determined. A promotion should trigger repo-gen as the regular build should be "newer" than all its draft builds. (But we could provide another approach to check that in the next iteration)

IMHO, if we don't have many draft builds in build-tags which probably burn kojira, there's no harm to record this action. WDYT?

As far as I can tell, these current changes do not address the issue of how to correctly map rpm components for buildroots and archives when nvras are no longer unique. This is arguably the most challenging aspect of the feature.

This need applies even in the code paths you have marked with reject_draft because these builds could still either have draft builds in their buildroots or as an image component.

True. I would let repo init function to handle that (by enriching pkglist file or something like this) for rpm components uniqueness in buildroot. The code is not perfect and introduces much complexity.

And the stakeholders want the draft building/promoting in the initial version of draft builds to be tried out and all other features can be postponed to the next round of implementation including draft builds within dependencies. So I think we could forbid draft builds from tagging to build-tag by 'tag' policy until it's well-programmed in the next iteration after some uses. WDYT?

I see that you've added a build opt to get_rpm, but it doesn't look like it's going to be used in these cases.

Yes, it is used in my draft code. getRPM API directly invokes it as well.

temporarily untagged from testing-ready to fix our QE build.

And the stakeholders want the draft building/promoting in the initial version of draft builds to be tried out and all other features can be postponed to the next round of implementation including draft builds within dependencies. So I think we could forbid draft builds from tagging to build-tag by 'tag' policy until it's well-programmed in the next iteration after some uses. WDYT?

As a stakeholder here, I agree. For the pilot phase of our project, we only need to be able to build a draft builds without any dependencies on other draft builds and promote them. For the future, dependencies between draft builds have to be possible, but we do not need it right now at the beginning.

I therefore agree that tagging Draft builds into build-tag should be disallowed for now and once properly implemented, enabled.

So I think we could forbid draft builds from tagging to build-tag by 'tag' policy until it's well-programmed in the next iteration after some uses. WDYT?

You don't have to tag a build into a build tag for it to be in a buildroot. Most content in buildroots is inherited into the build tag from other tags. Enforcing this with policy is going to be tricky and error-prone.

One thought that has occurred to me is to block this at the import step. That is, in the places where we are matching buildroot or archive components, we would simply not allow draft rpms to be listed. That would mean that only scratch builds could use draft builds in the buildroots. Such a restriction could later be softened, e.g. to allow such refs for other draft builds.

Regardless whether we allow it or not, we can't completely avoid the possibility of a draft build getting into a buildroot (or an image), so we must solve the mapping problem now.

I would let repo init function to handle that

I definitely agree that we'll need to have repo_init record the koji references (rpm, archive, and build ids) for the components that go into the repos. However, we'll also need other parts of the code to utilize that data to perform the mapping. This could probably be done in the BuildRoot class so that it can report rpm components by unambiguous rpm_id, though perhaps we also want a helper function on the hub to facilitate that mapping.

As a stakeholder here, I agree. For the pilot phase of our project, we only need to be able to build a draft builds without any dependencies on other draft builds and promote them. For the future, dependencies between draft builds have to be possible, but we do not need it right now at the beginning.

My main concern is that we don't want to paint ourselves into a corner. It's one thing to defer some implementation, but if we don't even know exactly what behavior we're deferring we could easily wind up in a situation where our end goal is incompatible with live data in Brew.

Also, given how common chain builds and bundled updates are, I find it hard to imagine we won't have developers complaining loudly if they can't use the draft build feature for a pair of dependent rpm builds.

In Koji, we already have a type of build that cannot be used in normal ways. That is the scratch build. If scratch builds were sufficient for the cases at hand, then we wouldn't have this feature request in the first place.

My view of draft builds so far has been that draft builds would work like regular builds in all ways except that:

1. they would have the draft flag set (and various policies could therefore work differently or them)
2. they have a specialized release value
3. their rpms are allowed to violate nvra uniqueness
4. they can be promoted to non-draft builds

Other than that, their build process would be exactly the same, and they would be allowed to be tagged (subject of course to tagging policy).

Draft builds are of course potentially promotable, and in principle no different than a non-scratch test build currently would be. This is ultimately a process convenience. The only real problem with having them in buildroots is the mapping issue (which we have to solve anyway in order to even know that we are dealing with a draft).

Some of the most complex changes here are in the manual rpm import code path, and I'm wondering if we can avoid some of this change for now. Is a clear requirement to support manual imports for draft builds?

This bit in the rpminfo cli handler:

srpminfo = session.listRPMs(buildID=info['build_id'], arches='src')[0]

I see why the change is needed and using listRPMs is a good idea. However, it is possible that this call could report an empty list (e.g. for manual import with no srpm).

This constraint seems redundant:

 CONSTRAINT draft_for_rpminfo UNIQUE (id, draft)

The id field is already unique, so why do we need this?

def append_draft_clause(draft, clauses, table=None): 

It would be better to simply have a call that returns the draft clause rather than assume that all callers want to append it to a list. E.g. like we do with name_or_id_clause and eventCondition.

target_release = draft_build.get('extra', {}).get('draft', {}).get('target_release')

Relying on extra for this seems wrong. Plus, we should be able to calculate the target release simply by stripping the draft suffix off (with sanity checks of course).

@mikem , for OSCI use-cases, I do not think manual imports of draft builds are needed. The only way how we want to build RPMs for RHEL is to build them in Koji.

Some of the most complex changes here are in the manual rpm import code path, and I'm wondering if we can avoid some of this change for now. Is a clear requirement to support manual imports for draft builds?

the changes in koji import code is to ease the testing for dev and qe. I think QE already covered them, but I'm ok to remove it now (It could be another PR for later review/testing)

Some of the most complex changes here are in the manual rpm import code path, and I'm wondering if we can avoid some of this change for now. Is a clear requirement to support manual imports for draft builds?

the changes in koji import code are to ease the testing for dev and qe. I think QE already covered them, but I'm ok to remove it now (It could be another PR for later review/testing)

This bit in the rpminfo cli handler:

srpminfo = session.listRPMs(buildID=info['build_id'], arches='src')[0]

I see why the change is needed and using listRPMs is a good idea. However, it is possible that this call could report an empty list (e.g. for manual import with no srpm).

Good catch, Thanks! I updated this a bit.

This constraint seems redundant:

CONSTRAINT draft_for_rpminfo UNIQUE (id, draft)

The id field is already unique, so why do we need this?

Yes, this is required by the foreign key in rpminfo table:

FOREIGN KEY (build_id, draft) REFERENCES build (id, draft) ON UPDATE CASCADE,

w/o the unique CONSTRAINT, the redundant (draft) with auto-update/delete can't be added there.

e.g:

koji=# alter table build drop CONSTRAINT draft_for_rpminfo cascade;
NOTICE:  drop cascades to constraint rpminfo_build_id_draft_fkey on table rpminfo
ALTER TABLE
koji=# alter table rpminfo ADD FOREIGN KEY (build_id, draft) REFERENCES build (id, draft) ON UPDATE CASCADE;
ERROR:  there is no unique constraint matching given keys for referenced table "build"

Added a comment there.

def append_draft_clause(draft, clauses, table=None):

It would be better to simply have a call that returns the draft clause rather than assume that all callers want to append it to a list. E.g. like we do with name_or_id_clause and eventCondition.

updated

target_release = draft_build.get('extra', {}).get('draft', {}).get('target_release')

Relying on extra for this seems wrong. Plus, we should be able to calculate the target release simply by stripping the draft suffix off (with sanity checks of course).

updated to parsing target release from draft release by a regex

the support of draft builds in cli handle_import is removed

Added some updates:
- ensuring nvra uniqueness in "standard" BuildRoot
- disallowing draft build in buildroot of CG
- disallowing draft rpms as rpm components of an image build
- disallowing draft in distRepo
- adding more draft checks for non-rpm build creation

@mikem Please take a look again

I'll have more to say (there's a lot of updates here), but let me start with the following.

• self._init_repo_data()
  • self.repodir is only used twice and kind of an odd value. I'm not sure it needs to be an instance variable.
  • so, maybe just leave that part out and rework the function as self.get_repo_url()
    • otoh, we only seem to use self.repo_url one place currently. maybe the code should go back there
• mapInternalRPMS checks os.path.exists(content_json_file), but this a relative path and is later used with openRemoteFile
  • this could should not assume that we have fs access to the repo
  • payloadhash mismatches should probably error
• also mapInternalRPMS prints a warning when the content file is missing, but then immediately error trying to open it
  • perhaps you meant to include a return here
  • I actually think we want this to be an error condition, at least eventually. However, there's a transitional issue when the change first rolls out.
  • We should at least note a TODO here
  • We could possibly have a fallback where the builder queries the repo content itself, perhaps just with draft=True
• I like having functions for adding and stripping the draft release, however, they seem more complex than they need to be.
  • They are written as if we intend to change the format over time. That is not the case. Changing the format in the future would be a backwards compatibility issue.
  • Note that the format #draft_{build_id} was chosen partially because # is not allowed in in release values by rpm.
• The changes in repo_init that convert the rpms generator to a complete list are concerning. This can be a lot of memory for some tags, and there are a lot or repoInit calls, all the time.
  • we can probably write this data out as newline-delimited json we read the generator, just as we write the pkglist file
  • we shouldn't expend too much effort to massage or index this data. The builders can do that.
  • also the field reduction here seems odd. We don't actually reduce space that much, and we omit the draft field
• There are numerous places where we're erroring on draft builds and it seems a bit much

While playing with these updates, I discovered a problem with the suffix format. #draft_12345 causes problems when the path is in a url. It breaks most rpm downloads, including in dnf. I had chosen this format so that no normal rpm could accidentally overlap. The non-alphanumeric characters that rpm allows here are:

#define ALLOWED_CHARS_VERREL "._+%{}~^"

Choosing a separator that:

• is not a valid character in an rpm release
• does not require special treatment in a url
• doesn't complicate shell commands

Doesn't leave a ton of options. I came up with the following list ­— : @ ,

Of these, I think , is probably the safest, but probably worth thinking more about.

This and other updates here -- https://pagure.io/fork/mikem/koji/commits/pr3913updates

@mikem I rebased your updates and made some updates too. PTAL again.

(Trying to remove some reject_draft calls)

removed some references of reject_draft in HostExports and new_typed_build

gen_draft_release masks the id builtin. We have very old code that erroneously does this a couple other places, but no need to repeat the mistake.

Does completeBuild need a draft option? It would be simpler not to pass it. We've already declared the build as a draft in initBuild.

The error the user sees when promoting a duplicate draft is pretty unreadable.

Access checks for promoteBuild is weirdly spread out. We check only for login in the wrapper, then rely on a policy check in _promote_build to enforce the intended access control. Better to manage access checks at the same level.

Allowing a user arg for _promote_build seems questionable. Plus it doesn't look like anything uses it. Tacitly relying on get_user(None) returning the logged in user without a clarifying comment is dangerous.

The strict arg for promote_build is similarly unused, and I am not convinced it is needed.

BuildRoot._setList looks for a build field, but we're adding an (rpm) id field in mapInternalRPMs. I don't know if we need that adjustment here. The get_rpm function will already use that id field.

reject_draft takes a strict option that we never use. The code would be simpler without it.

_clean_draft_link should handle the case where a build changes volumes after being promoted, or even has been moved across multiple volumes.

on that subject, _promote_build should probably handle the symlink a little more carefully. It's safest to always symlink to the DEFAULT volume (as _set_build_volume does when moving volumes), since there should always be a link there to whatever volume the build is on.

Also another update here -- https://pagure.io/fork/mikem/koji/commits/pr3913updates

I've added a rework of get_rpm on my branch as well

https://pagure.io/fork/mikem/koji/commits/pr3913updates

A few more updates posted to my branch

The last update is probably worth a note. Once we've promoted a draft, it seems like we shouldn't allow further draft builds for that nvr. The won't be promotable, and the recycling logic doesn't apply.

Thanks, @mikem,

I merged your updates to this PR, added a note in import_rpm, and fixed tests

I've added more updates on my branch ­— https://pagure.io/fork/mikem/koji/commits/pr3913updates

These address the last few things that were really bugging me about these changes. Mainly, using build.extra to track core build data.

If this looks ok, then we can move forward with merging and testing.

Thanks, @mikem

The changes look good to me.

Commit 75a3cab1 fixes this pull-request