search

sigstore / sigstore-rs

An experimental Rust crate for sigstore
https://sigstore.github.io/sigstore-rs/sigstore/
Apache License 2.0
156 stars 48 forks source link

The `Repository` trait and `ManualRepository` struct no longer require a feature flag #331

Closed tannaurus closed 3 months ago

tannaurus commented 4 months ago

Summary

These changes just move the Repository trait and ManualRespository struct out from behind of the tuf feature flag. I've updated all tests and examples to point to the new repository module and have re-exported both items from tuf to prevent breaking changes.

Primary motivations

  1. The cosign feature flag is no longer dependent on tuf feature flag. This addresses the issue I've linked to this MR.
  2. The tuf feature flag, renamed in this PR to sigstore-trust-root, pulls in the tough and regex dependencies. These dependencies are not needed by the ManualRepository struct, renamed to ManualTrustRoot in this PR, leading to a bloated binary size. More critically, tough is not wasm compatible: it depends on path-absolute which depends on path-dedot which is not wasm compatible. Someone who wishes to use ManualRepository and compile to a wasm target currently cannot do so.

Closes: https://github.com/sigstore/sigstore-rs/issues/318

Renamed structs, feature flag, modules, and trait

An independent issue came up while the initial commits were being reviewed: the names of the tuf feature flag and the naming convention of "Repository" throughout the impacted modules. These changes also seek to address this feedback. The following changes were made:

Release Note

Documentation

n/a imo. Happy to fill in gaps where you reviewers feel it necessary.

tannaurus commented 4 months ago

Hi @tannaurus Thanks for the fix and it really makes sense where we do not need to rely on tough but only cosign.

Based on your refactoring, I want to share some more. Respository can be some default code of sigstore-rs without specifying a feature repository as it does not import any new dependencies.

At the same time, a new feature sigstore-repository to replace tuf, which will enable tough and regex and implementations of SigstoreRepository.

This would make the code and names more clear and straightforward. Wdyt?

I really like the idea of removing the repository feature flag!

I'm a bit more hesitant to get on board with changing the tuf feature flag's name, only because it would be a breaking change. Changing it would no doubt improve the discoverability of the feature, but it would require downstream dependents with default features disabled to update their cargo file. We'd also probably want to change the module name too: it's still named tuf and is the only module in lib.rs that's name doesn't match the feature flag. This of course would be a larger breaking change.

I've pushed a commit implements the change, but I'm personally in favor of leaving it as tuf. I'd defer the final decision to the maintainers of this repo. Should there be support for renaming the feature flag, I'd be happy to push an additional commit that renames the module; if there's support for that too.

tnytown commented 4 months ago

Hey @tannaurus! thanks for fixing this fallout from some of our cosign changes 😅

Extremely minor bikeshed: I wonder if there's a clearer name for the repository module and its types. When we defined ManualRepository in #305, we decided to inherit the naming scheme from the existing SigstoreRepository; though now that I think of it, Repository doesn't completely capture their purpose. Maybe something like TrustRoot would be more appropriate?

Of course, renaming all these things would be a breaking change, and I'll also defer judgement to the maintainers. Food for thought :)

tannaurus commented 4 months ago

Hey @tannaurus! thanks for fixing this fallout from some of our cosign changes 😅

Extremely minor bikeshed: I wonder if there's a clearer name for the repository module and its types. When we defined ManualRepository in #305, we decided to inherit the naming scheme from the existing SigstoreRepository; though now that I think of it, Repository doesn't completely capture their purpose. Maybe something like TrustRoot would be more appropriate?

Of course, renaming all these things would be a breaking change, and I'll also defer judgement to the maintainers. Food for thought :)

I'm all for bikeshedding like this! I agree with you. "Repository" does not feel intuitive to me either. This does feel like something that should be addressed in a follow up issue though, what do you think? We can keep the discussions around module/feature flag names in that issue and make the refactors once there is buy in from the maintainers.

I've gone ahead and reverted the feature flag changes suggested by @Xynnn007 in an effort to keep these changes truer to its initial goal. I like these suggestions and would be happy to implement them in a different PR.

tnytown commented 4 months ago

I'm all for bikeshedding like this! I agree with you. "Repository" does not feel intuitive to me either. This does feel like something that should be addressed in a follow up issue though, what do you think? We can keep the discussions around module/feature flag names in that issue and make the refactors once there is buy in from the maintainers.

Of course! I have no urgent need for the rename :)

Xynnn007 commented 4 months ago

cc @flavio @viccuad

flavio commented 4 months ago

I'm fine changing the name of the feature, I'm not too afraid of breaking downstream users.

I'm also in favor of renaming Repository and ManualRepository to something like @tnytown proposed. I don't find these names helpful.

tannaurus commented 4 months ago

I'm fine changing the name of the feature, I'm not too afraid of breaking downstream users.

I'm also in favor of renaming Repository and ManualRepository to something like @tnytown proposed. I don't find these names helpful.

Works for me!

I've renamed the following:

Thoughts? cc @tnytown @Xynnn007

tnytown commented 4 months ago

I've renamed the following:

  • Module repo (introduced in this MR) is now trust
  • Module tuf is now sigstore and is a sub module of trust
  • Trait Repository is now TrustRoot
  • Struct ManualRepository is now ManualTrustRoot
  • Struct SigstoreRepository is now SigstoreTrustRoot
  • Feature flag tuf is now sigstore-trust-root

Looks good to me, thanks @tannaurus!

tannaurus commented 4 months ago

Thanks @Xynnn007 and @tnytown !

I've went ahead and updated the PR description to capture these changes in their entirety.

@flavio if there's anything you'd like changed here I'm happy to do it, but otherwise I believe this is ready to go 🙂

Xynnn007 commented 3 months ago

@tannaurus Hi, the DCO check fails. You might need to add sign-off-by for each commit message. A relative link https://cert-manager.io/docs/contributing/sign-off/ might help

tannaurus commented 3 months ago

@tannaurus Hi, the DCO check fails. You might need to add sign-off-by for each commit message. A relative link https://cert-manager.io/docs/contributing/sign-off/ might help

Ah, forgive me 👀 I missed that. I've just rebased my changes (git rebase --signoff main) and forced pushed those commits up to his branch.

tannaurus commented 3 months ago

I'm a bit perplexed by the failing checks, they don't appear to be related the changes I've made here. It looks like Github's "Unchanged files with check annotations" feature is caught me in a snare. Could one of the maintainer's advise me towards a path forward? I'd be happy to push up the suggestions it is requesting, just feels a bit odd to be doing that in this PR.

Xynnn007 commented 3 months ago

@tannaurus Yes. The lint error will be fixed by https://github.com/sigstore/sigstore-rs/pull/334

Let us get that merged quickly and you can rebase this PR. cc @flavio

tannaurus commented 3 months ago

Looks like that did the trick! I believe this can be merged by someone who is authorized 🙂