Merge lp://staging/~fginther/ubuntu-ci-services-itself/lander-binary-fixes into lp://staging/ubuntu-ci-services-itself

FAILED: Continuous integration, rev:290
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/266/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/266/rebuild

46 + packages = request_parameters.get('added_binaries', '')
47 + if packages:
48 + packages = packages.split(',')

I don't think you need 47 & 48, python's split should be safe there.

63 + parameters = {'added_binaries': ','.join(packages),
64 + 'removed_binaries': None}

Bikeshedding, but most of our code is declaring dictionaries styled like:

  parameters = {
      'added_binaries': ','.join(packages),
      'removed_binaries': None
  }

20 + if added_list:
21 + for package_name in added_list.split(','):
22 + if package_name not in package_list:
23 + # Only add packages not already in the global list
24 + package_list.append(package_name)
25 +
26 + # Finally remove the list of removed binaries from the ticket request
27 + removed_list = request_parameters.get('removed_binaries', '')
28 + if removed_list:
29 + for package_name in removed_list.split(','):
30 + try:
31 + package_list.remove(package_name)
32 + except ValueError:
33 + # This is ok, multiple queued tickets could be trying
34 + # to remove the same binary.
35 + logger.info('Package to remove not found: '
36 + '{}'.format(package_list))
37 +

Not a big deal, and I could see some people advocate what you have there. But I often find it easier to use Python "sets" for things like this. I think you could simplify this to something like:

 packages = set(ticket.get_binaries())
 packages = packages + set(request_parameters.get('added_binaries', '').split(','))
 packages = packages - set(request_parameters.get('removed_binaries', '').split(','))

None of my comments are really a big deal as the code looks like it works to me. I'll just ack the MP and you can decide whether or not you want to do anything with my comments.

PASSED: Continuous integration, rev:290
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/273/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/273/rebuild

> 46 + packages = request_parameters.get('added_binaries', '')
> 47 + if packages:
> 48 + packages = packages.split(',')
>
> I don't think you need 47 & 48, python's split should be safe there.

added_binaries and removed_binaries can be defined as None in which case "AttributeError: 'NoneType' object has no attribute 'split'" can be raised.

> 63 + parameters = {'added_binaries': ','.join(packages),
> 64 + 'removed_binaries': None}
>
> Bikeshedding, but most of our code is declaring dictionaries styled like:
>
> parameters = {
> 'added_binaries': ','.join(packages),
> 'removed_binaries': None
> }

I wasn't aware of this, I can switch.

> 20 + if added_list:
> 21 + for package_name in added_list.split(','):
> 22 + if package_name not in package_list:
> 23 + # Only add packages not already in the global list
> 24 + package_list.append(package_name)
> 25 +
> 26 + # Finally remove the list of removed binaries from the ticket
> request
> 27 + removed_list = request_parameters.get('removed_binaries', '')
> 28 + if removed_list:
> 29 + for package_name in removed_list.split(','):
> 30 + try:
> 31 + package_list.remove(package_name)
> 32 + except ValueError:
> 33 + # This is ok, multiple queued tickets could be trying
> 34 + # to remove the same binary.
> 35 + logger.info('Package to remove not found: '
> 36 + '{}'.format(package_list))
> 37 +
>
> Not a big deal, and I could see some people advocate what you have there. But
> I often find it easier to use Python "sets" for things like this. I think you
> could simplify this to something like:
>
> packages = set(ticket.get_binaries())
> packages = packages + set(request_parameters.get('added_binaries',
> '').split(','))
> packages = packages - set(request_parameters.get('removed_binaries',
> '').split(','))

I like your recommendation here, it's much cleaner. Unit tests are in place, so it's an easy change to test.

> None of my comments are really a big deal as the code looks like it works to
> me. I'll just ack the MP and you can decide whether or not you want to do
> anything with my comments.

PASSED: Continuous integration, rev:290
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/275/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/275/rebuild

PASSED: Continuous integration, rev:291
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/278/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/278/rebuild

Andy, these changes should address your comments.

 merge approved