Merge lp://staging/~sinzui/juju-release-tools/validate-streams into lp://staging/juju-release-tools

I am finding it very hard to understand check_expected_tools. I don't know why it will accept version and then basically ignore it if retracted is set. It seems like retracted is really a mode, not a parameter. I would like to see a test like this:

old_tools = make_tools_data('trusty', 'amd64', ['1.20.7', '1.20.9'])
new_tools = make_tools_data('trusty', 'amd64', ['1.20.7', '1.20.8'])
check_expected_tools(old_tools, new_tools, '1.20.8', '1.20.9')

Notice that 1.20.8 has been added and 1.20.9 has been removed. I don't know what the expected behaviour is. I think the actual behaviour is that it will complain that 1.20.8 is unexpected.

Hi Aaron. Thank you for the review. your question has raised a scenario I had not considered.

Retract is like a mode. We are removing a set instead of adding one. The version being published is likely to be the previous version. The version is always passed because Jerf will always be installing that version to create the streams. I assumed that the version is the previous good version.

I don't think your example is real. I cannot think of retracting a later version, and publishing an earlier non-existent version, but...

I can image waking up and find an email from Ian that says there is a security issue with version n, his team have prepared version q. We need to retract earlier n and since something is ready, publish later q. I would certainly want to do one publication, not two to achieve this. I am certain this test would fail because there are two expected differences.

old_tools = make_tools_data('trusty', 'amd64', ['1.20.7', '1.20.8'])
new_tools = make_tools_data('trusty', 'amd64', ['1.20.7', '1.20.9'])
check_expected_tools(old_tools, new_tools, '1.20.9', '1.20.8')

I am going to change the rules to accommodate this case. This is tricky I think the rule is that when doing a retractions, we need to ask if the version being published is new and need to be included in the expected differences.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 14-10-17 01:58 PM, Curtis Hovey wrote:
> I am going to change the rules to accommodate this case. This is
> tricky I think the rule is that when doing a retractions, we need
> to ask if the version being published is new and need to be
> included in the expected differences.

I don't think it's actually that tricky. I think it's something like

added = dict(t for t in new_tools.items() if t[0] not in old_tools)
wrongly_added = dict(t for t in added.items()
                     if t[1][version] != new_version)
removed = dict(t for t in old_tools.items() if t[0] not in new_tools)
if retracted is None:
    wrongly_removed = removed
else:
    wrongly_removed = dict(t for t in added.items()
                           if t[1][version] != retracted)

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUQWVfAAoJEK84cMOcf+9hk5QIAIk9rRS3YnrPjwSrhsRHB8n9
FXgZPXTi/Z61AbG3nrbESbpHE8R8FVtM5dQL7chVbhUSi52oFl2CVVS4wBTfX+/r
TYViz/bJQOTuAAgOwtRIMZ/ylHSdgjgVcbI0AaOguP5KevuERWxR3mXH6pusdX5+
xeaYtCTkkgK2jfcBNdPW54o97T/MnCh57iRKOFmgWCm5QMktizGkyPAvgpCBwhmx
vaPwjqnFjZUIuN3zWLFL7Gn6YS9QHWWbDAvjKp2XqyoDAuKVk3x+QH5RhFkuQlfB
luf56rgHhj/YoCbkBrj4dB6gTcv2pmFW1mHdontpKJvn2bgcOXK5tKIpYVqQBuw=
=Sd2G
-----END PGP SIGNATURE-----

I have a subtle change to the script. Instead of passing the args that might be passed to assemble-streams, the script now expects the caller to clearly state the expected differences.

I changed the script command line to clearly indicate when a version is added:
   --added 1.20.9 --retracted 1.20.8

Which also renamed version => added

I have changed added to be optional so that the function does not need to guess about expected differences
    check_expected_tools(old_tools, new_tools, added=None, retracted=None)

The assemble-streams script will need intelligence to know when something is added and retracted. This might not be tricky since it is configured with the reacted version and there is a check if any new tools were created.

Note I made a change to verbose. I used this script for the 1.20.10 release, and I had to check the return code because --verbose didn't clearly tell me that everything was fine. I really wanted a clear message that the content was correct.

Oh, and I am sure you noted that temp_dir() could be extracted to a utils.py. I have done this to keep the code DRY.

Okay, I think I'm getting the hang of this code, and I'm pretty sure it is not correct. There are two ways check_expected_tools can be tricked out of warning that "added" is missing.

1. Do a successful retract. This means that expected_differences will be non-empty from the retract clause, and so the added clause cannot set missing_errors.

2. Have other missing versions. This means that the "added" clause will set missing_errors, but the "missing" clause will overwrite its value. There will be an error, but it won't mention "added".

Other comments below.

I have replied to the comments and pushed up some changes. I have another change I need to make, at least a test, and maybe a code change to account for streams.canonical.com's behaviour.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 14-10-20 05:55 PM, Curtis Hovey wrote:
> The "versions" keys are arbitrary names :(. juju metadata will
> name it something like "20141020". We only get 1 key in versions,
> but the specification allows many. We don't care if there is 1 or
> many of these versions we only want to get the the next level,
> "items". The goal is to say all items on old are the same as the
> items in new, except we know that we sometimes add items and we
> have one removed items, once published without items, and on a few
> occasions change items.
>
> In Xpath, this is products/versions/*/items. I could change the
> test for items to be if isinstance(version, dict) and "items" in
> versions:

Okay, the code is fine as it stands, then.

>> + # needs a version to install to make streams, even when
>> it + # intends to remove something. + for n, t in
>> old_expected.items(): + if t['version'] == retracted:
>> + expected_differences.update([(n, t)])
>
> The goal was to state the expected differences to create a common
> set of old and new tools that must match in name a content.

Deleting entries from old_expected is perfectly sane. But why are
updating expected_differences? You don't use it anywhere, except the
"added" clause. And in the "added" clause, it can only cause a bug,
by falsely preventing the "missing.append(added)".

> So expected_differences was just a way to remove what we know to
> be different to prove nothing else is changed.

No, it doesn't do anything like that. All it does is prevent
"missing.append(added)", so that this version is not considered missing.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJURnGpAAoJEK84cMOcf+9hwDkIAJmJOGytVsStF5De6MijLpV5
4G6YNYWhzp+ShM38KIhT39CZ83pJV4DY7yO3ajIi03/359j1rufRug4CxukxPGwq
ByQO+/SuyvKZYpHFRVqmpsCe1kaKX4WC8gInLm2JLgFoMr/zXDWAL9jnZ8TMmEa7
V0a9T/jNit/3q9t4hMEoaNohaAaWj3vxbNWdm/gasmtApMiqtLltHD1SNEm8czau
py1qmtidjNd4R4q+TTjROmo5uJqOR0jPBOe1MoYdzzUY4F/1QgqWlCEo6TEVTseU
rSuaU1aVG0ks63Et+S8/GcBuqGOjT5DIjHDIt1LYVPa4xZYuBkHuP6loJmEyOZ4=
=GrUS
-----END PGP SIGNATURE-----

My latest changes follow your suggestion to simplify the purpose of the functions to make the code easier to maintain. I split the rules to check for expected versions from rules to find unexpected version.

I also changed all the checks to always return a list to make the contact simple and it is easy for the callee to extend the list of errors.

This looks clear and correct. Thanks!