#3942 policy_data_from_task_args: set target to None when it doesn't exist
Merged 6 months ago by tkopecek. Opened 8 months ago by julian8628.
julian8628/koji target-with-repoid  into  master

file modified
+9 -5
@@ -9889,7 +9889,6 @@ 

              policy_data['source'] = params.get(k)

              break

      # parameters that indicate build target

-     target = None

      hastarget = False

      for k in ('target', 'build_target', 'target_info'):

          if k in params:
@@ -9903,10 +9902,15 @@ 

                  target = None

              else:

                  target = target.get('name')

-         if target is None:

-             policy_data['target'] = None

-         else:

-             policy_data['target'] = get_build_target(target, strict=True)['name']

+         if target is not None:

+             tinfo = lookup_build_target(target, strict=False)

+             if tinfo is None:

+                 logger.warning("No such build target: %s", target)

+                 target = None

+             else:

+                 target = tinfo['name']

+         policy_data['target'] = target

+ 

      t_opts = params.get('opts', {})

      policy_data['scratch'] = t_opts.get('scratch', False)

  

for build task, the build target could be destination tag when opts.repo_id is specified

fixes #3941

elif method == 'build' and params.get('opts', {}).get('repo_id'):
    # target is actually destination tag here

This is not a given.

There is a case where the target field can be interpreted as a destination tag, but it is not quite this simple. The presence of the repo_id option is not sufficient to determine this. The handler always tries to treat the argument as a target first.

I wonder if this is the right place to fix this. Would be helpful to have more detail on the actual problem

Instead I think we should check the result of get_build_target first. if it is not a valid target, report target as none. This is closer to the logic in the task handler

rebased onto c2a0523d7d28518ca3b6a23f1ca1834dd6fbf7fe

8 months ago

Instead I think we should check the result of get_build_target first. if it is not a valid target, report target as none. This is closer to the logic in the task handler

Ah, that's right, I changed it like this

rebased onto f25f20e2c754d7d4822e953a0c72db31fa31bbf6

8 months ago

hmmm.. it's probably too simple. maybe better to get event from repo_id

1 new commit added

  • get target with event when it's available
8 months ago

1 new commit added

  • get target with event when it's available
8 months ago

2 new commits added

  • get target with event when it's available
  • policy_data_from_task_args: set target in policy_data to None if it doesn't exist
8 months ago

added event with repo_id

2 new commits added

  • get target with event when it's available
  • policy_data_from_task_args: set target in policy_data to None if it doesn't exist
8 months ago
if t_opts.get('repo_id') is not None:
    rinfo = repo_info(t_opts['repo_id'])
event = rinfo['create_event'] if rinfo else None

rinfo could be undefined here

It would probably be a good idea to log a warning if the task has a non-None target specification that we cannot map to a target.

Previously we only added target=None to the policy data if the task had an explicit target of None. Now we will also report None when there is a target value, but we can't map it. Hence a good idea to warn.

It might be better to use lookup_build_target rather get get_build_target. This will work even if the target has been deleted (which could be the case if we're called from apply_volume_policy). If we do, I think we can skip worrying about the repo id since we won't need an event arg.

rebased onto 6d15e5ff7e641100ff6f1c6e5a3d2b9c458e2522

8 months ago

Updated with the approach of using lookup_build_target instead of get_build_target, and added a warning about "Target not found". Please take a look again.

if target is not None:
    ...
else:
    # explicit None
    target = None

The else case is redundant as we only reach it when target is None.

logger.warning("Target not found: %s (inluding deleted)", target)

We don't need to specify including deleted. It would be better to be consistent with other similar messages/errors and simply have: "No such build target: %s"

rebased onto 3c25960

7 months ago

Metadata Update from @julian8628:
- Pull-request tagged with: testing-ready

7 months ago

Metadata Update from @relias-redhat:
- Pull-request tagged with: testing-done

6 months ago

Commit aa94ecb fixes this pull-request

Pull-Request has been merged by tkopecek

6 months ago