juju-release-tools

Code review comment for lp:~sinzui/juju-release-tools/validate-streams

validate-streams
Merge into trunk

Revision history for this message

Aaron Bentley (abentley) wrote on 2014-10-21:

-----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-----

-----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

« Back to merge proposal