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

command to purge all keys from all delegations below a starting point #855

Merged
merged 3 commits into from
Jul 29, 2016

Conversation

endophage
Copy link
Contributor

@endophage endophage commented Jul 20, 2016

Need to write tests.

Closes #696

Signed-off-by: David Lawrence david.lawrence@docker.com (github: endophage)

@endophage endophage added this to the Notary 0.4 milestone Jul 20, 2016
@endophage endophage force-pushed the delegation_remove_key branch 6 times, most recently from 80ba1f0 to abc8943 Compare July 21, 2016 20:50
@endophage endophage changed the title WIP: command to purge all keys from all delegations below a starting point command to purge all keys from all delegations below a starting point Jul 21, 2016
@endophage
Copy link
Contributor Author

@cyli @riyazdf ready for review

path.Join(CanonicalTargetsRole, "level1"): tr,
path.Join(CanonicalTargetsRole, "level1", "level2", "level3"): tr,
path.Join(CanonicalTargetsRole, "under_score"): tr,
path.Join(CanonicalTargetsRole, "hyphen-hyphen"): tr,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this refactoring 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted consistency within the file :-)

@endophage endophage force-pushed the delegation_remove_key branch 3 times, most recently from 4d25d99 to 34606c0 Compare July 24, 2016 21:50
path.Join(CanonicalSnapshotRole, "*"): f,
path.Join(CanonicalTimestampRole, "*"): f,
path.Join(CanonicalTargetsRole, "*", "foo"): f,
path.Join(CanonicalTargetsRole, "*", "*"): f,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can also add failure cases for trailing slashes after the *, and extra slashes between the rest of the delegation and the *?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

David Lawrence added 2 commits July 28, 2016 16:27
Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)
Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)
@riyazdf
Copy link
Contributor

riyazdf commented Jul 29, 2016

thank you for adding the tests, LGTM! Documentation CI errors are unrelated

@@ -92,6 +91,10 @@ func changeTargetsDelegation(repo *tuf.Repo, c changelist.Change) error {
if err != nil {
return err
}
if data.IsWildDelegation(c.Scope()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is never getting exercised, which is weird because cmd/notary/integration_test.go's TestPurge should exercise this bit of code.

In applyChangelist, where we switch based on whether the scope is a delegation (L43) I think we also have to change that line to be isDel := data.IsDelegation(c.Scope()) || data.IsWildDelegation(c.Scope())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, you're absolutely right, and that's making me wonder why the test in cmd/notary/delegations_test.go is even passing.

Going to add some more checks to that test to try and make it fail, then will update this and see if it passes again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see now. I'm calling the purge command but not publish so it's not actually applying the change. I must have got sidetracked half way through the test and forgotten to come back to it.

Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)
@cyli
Copy link
Contributor

cyli commented Jul 29, 2016

Awesome, thanks for tackling this! LGTM!

@cyli cyli merged commit cf6c8b0 into notaryproject:master Jul 29, 2016
@endophage endophage deleted the delegation_remove_key branch July 29, 2016 22:49
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.

4 participants