Skip to content
This repository was archived by the owner on Jul 31, 2025. It is now read-only.

Conversation

riyazdf
Copy link
Contributor

@riyazdf riyazdf commented Jul 21, 2016

Brought up by @HuKeping in #852 (comment), I noticed that translateDelegationsToCanonicalIDs was mistakenly editing in-memory metadata by replacing TUF key IDs with canonical key IDs, which would cause nested delegations to have issues when building into data.DelegationRole structs.

Changed pass-by-reference to pass-by-value and added a test that publishes a nested delegation and ensures that delegation list behaves as expected.

@riyazdf riyazdf changed the title Ensure what we do not clobber local key id data for nested delegation… Ensure what we do not clobber local key id data on delegation list Jul 21, 2016
@riyazdf riyazdf changed the title Ensure what we do not clobber local key id data on delegation list Ensure what we do not clobber in-memory key id data on delegation list Jul 21, 2016
@HuKeping
Copy link
Contributor

I personally tried this and it works as expected, thanks for the quick fixing!

@riyazdf
Copy link
Contributor Author

riyazdf commented Jul 21, 2016

@HuKeping thank you for trying this out 👍

@cyli
Copy link
Contributor

cyli commented Jul 21, 2016

Great catch! Thanks for fixing this so fast, and for the test! Non-blocking, but would it make sense to add a moe specific unit test for GetDelegationRoles to ensure that it does not alter any of the repo metadata?

Otherwise LGTM!

@riyazdf riyazdf changed the title Ensure what we do not clobber in-memory key id data on delegation list Ensure that we do not clobber in-memory key id data on delegation list Jul 21, 2016
@riyazdf riyazdf added this to the Notary 0.4 milestone Jul 21, 2016
@HuKeping
Copy link
Contributor

Pre-expansion patch live now :) @cyli

@riyazdf riyazdf force-pushed the fix-deep-delegation-listing branch from f76839c to b828631 Compare July 22, 2016 00:31
when listing and converting to canonical ID

Signed-off-by: Riyaz Faizullabhoy <[email protected]>
@riyazdf
Copy link
Contributor Author

riyazdf commented Jul 22, 2016

@cyli: great idea, added on to a delegation publishing test

@endophage
Copy link
Contributor

LGTM

@endophage endophage merged commit c4f5da0 into master Jul 25, 2016
@endophage endophage deleted the fix-deep-delegation-listing branch July 25, 2016 20:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants