Skip to content

Add new Client constructors, deprecate old ones #242

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

Merged
merged 4 commits into from
Dec 7, 2019

Conversation

erickt
Copy link
Collaborator

@erickt erickt commented Nov 28, 2019

This patch changes how Clients are created. It adds the following
functions:

  • Client::from_local - use the specified root version from the local repository as our initial trusted root.
  • Client::from_pinned_root_keyids - use the specified root version, threshold, and keyids to trust a root fetched from the local or remote repository.
  • Client::from_pinned_root_keys - use the specified root version, threshold, and public keys to trust a root fetched from the local or remote repository.
  • Client::from_pinned_root - use the specified root metadata as the initial trusted root.

This deprecates the old constructors:

  • Client::new
  • Client::with_root_pinned

Closes: #229

@erickt erickt requested a review from heartsucker November 28, 2019 01:16
@erickt erickt force-pushed the init2 branch 2 times, most recently from a519604 to d5a51f5 Compare December 2, 2019 23:53
@erickt erickt requested a review from ComputerDruid December 3, 2019 18:35
This patch changes how Clients are created. It adds the following
functions:

* `Client::from_local` - use the specified root version from the local
  repository as our initial trusted root.
* `Client::from_pinned_root_keyids` - use the specified root version,
  threshold, and keyids to trust a root fetched from the local or remote
  repository.
* `Client::from_pinned_root_keys` - use the specified root version,
  threshold, and public keys to trust a root fetched from the local or
  remote repository.
* `Client::from_pinned_root` - use the specified root metadata as the
  initial trusted root.

This deprecates the old constructors:

* `Client::new`
* `Client::with_root_pinned`

Closes: theupdateframework#229
Copy link
Collaborator

@ComputerDruid ComputerDruid left a comment

Choose a reason for hiding this comment

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

FYI, from cargo rustdoc:

warning: `[Client::from_root_pinned_keyids]` cannot be resolved, ignoring it...
   --> src/client.rs:154:54
    |
154 |     /// **DEPRECATED**: This has been replaced with [Client::from_root_pinned_keyids].
    |                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot be resolved, ignoring
    |
    = note: `#[warn(intra_doc_link_resolution_failure)]` on by default
    = help: to escape `[` and `]` characters, just add '\' before them like `\[` or `\]`

    Finished dev [unoptimized + debuginfo] target(s) in 0.11s

I can't seem to comment on the right line because the diff isn't loading, not sure what's up with that.

@erickt
Copy link
Collaborator Author

erickt commented Dec 3, 2019

Thanks, I just pushed up a fix for the deprecation notice. It also looks like the diff view is working again.

src/client.rs Outdated
Comment on lines 439 to 443
// Also store this root metadata as the latest version.
local
.store_metadata(&root_path, &MetadataVersion::None, &root)
.await?;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be conditional on whether you're increasing the version somehow. Can you add tests for this case to each of these constructors to show that these don't roll back root?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it was a mistake to fold this change into this patch. I'll remove it.

* renamed `Client::from_*` to `with_*` and `Tuf::from_*` to `Tuf::with_*`.
* renmaed `Client::from_local` to `Client::from_trusted_local` to be a
  little more explicit.
* Don't write unversioned root metadata in the constructor
* Don't write root metadata in `Client::with_pinned_root`
* rename fn to `fetch_metadata_from_local_or_else_remote`
* ran `cargo fmt
* added a few more tests
@erickt erickt requested a review from ComputerDruid December 4, 2019 04:29
Copy link
Collaborator

@ComputerDruid ComputerDruid left a comment

Choose a reason for hiding this comment

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

Overall looks good but I think you could improve the naming of these constructors.

Change-Id: Ia42ac5ba7a05ea3cde64047cd8cf54ac0950cac2
@erickt erickt merged commit 43edf2e into theupdateframework:develop Dec 7, 2019
@erickt erickt deleted the init2 branch December 7, 2019 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Client::with_root_pinned should take public keys, not key ids
2 participants