I'm wondering if it makes sense to use auto also for tag and fromtag tests. As after deletion there is no longer active record in tag_packages I would incline to not adding it there. @mikem @julian8628 ?

Agree, I think the target of tag and fromtag tests should always exist. It's different from the buildtag test

The part of this that is actually related to #2917 (passing event=auto) is not reflected in the description of the PR.

This won't fix the issue for the buildtag_inherits_from test, but I guess (?) there might be some cases where the buildtag test could fail on a deleted tag.

Looking at this code again, it appears we're still not completely honoring the taginfo flag. When we get the to the part where we look at buildroots, we currently do:

tinfo = get_buildroot(br_id, strict=True)
tags[tinfo['tag_name']] = tinfo

When means in the taginfo=True case, we're going to return a buildroot dict instead of a taginfo one.

I've fixed the buildroot part. What remains is how exactly query inheritance for deleted tag. Does it make sense (as in last commit) to query inheritance from the last known state? Or should we look into the moment when build was done? I don't find the expected semantics in this case quite clear.

Yeah, the event thing is kind of a mess. Consider the cases in policy_get_build_tags...

When policy data has an explicit build tag, then event="auto" seems correct.

When policy data has an explicit target, we're checking for that target at current event too. I guess that could also fail the same way, but I think this only happens when handling a current task (either in channel policy or a policy checked by the builder). Either way, we can reasonably expect the target to exist. That's a long way of saying that "auto" is reasonable here.

When we're looking up the buildroots, we should probably actually use the repo_create_event_id for that buildroot instead.

Side note: there's a lot of repetition in policy_get_build_tags. I wonder if we could clean that up a bit, but that's not a big priority.

Meanwhile in buildtag_inherits_from, we get our tag list from policy_get_build_tags. In principle I think, we want to query inheritance the same way that we queried the tag. That's probably going to be messy to propagate.

I don't think we need to add the "auto" option to readFullInheritance. If the tag came from an event=auto query, then we can just use event=tinfo.get('revoke_event') as you have here, but if the tag came from a specific event query, as I suggest for the buildroot case, then we probably want that same event. We could just stick an extra field in the taginfos returned by policy_get_build_tags to propagate it, or we could possibly even have get_tag record query_event field.

The get_tag() change looks good.

With the changes to policy_get_build_tags, we're changing the priority of data sources. Previously, we relied on checking different cases and returning when we found a match. Now that we're not returning, we will always go through all the checks, but the last matching one will win instead of the first matching one.

It might not matter much, but it does seem like a "build_tags" field in the policy data should take precedence over target, which should similarly take precedence over digging into buildroots.

Perhaps just wrap each of these later cases in if not tags so they are only checked if we haven't found a match yet.

in BuildTagInheritsFromTest, we check for tinfo.get('query_event') == 'auto', but that field will never be "auto". In the auto case we're storing the specific chosen numeric event that was used. We can probably just simplify this to using the numeric query_event if it is there.

Also the if 'build_tag' in data case should not be a special return. The policy_get_build_tags function is only called two places and and both assume the result is a list of tags.

Fixed, I wonder if original 'build_tag was ever used in any test.

Fixed, I wonder if original build_tag was ever used in any test

It's given in policy data by the build task when checking the build_from_srpm and
build_from_repo_id. OTOH, it looks like the build_tags case is not used, at least by Koji itself.

Looks like I added the build_tags case when the policy_get_build_tags logic was first split out for the volume policy work. Before that, the logic was directly in BuildTagTest.

Maybe an interim change in the volume policy work actually did set build_tags, or maybe this was just for completeness.

At any rate, current changes look good!

:thumbsup:

Commit 9a3f9d48 fixes this pull-request