Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that we do not clobber in-memory key id data on delegation list #864

Merged
merged 2 commits into from
Jul 25, 2016

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.

…s on list

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>
@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 <riyaz.faizullabhoy@docker.com>
@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 join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants