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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 18 commits into from
Mar 12, 2024
Merged

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

merged 18 commits into from
Mar 12, 2024

Conversation

tannaurus
Copy link
Contributor

@tannaurus tannaurus commented Feb 26, 2024 •
edited
Loading

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: #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:

  • 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

Release Note

  • Module tuf is now sigstore and is a sub module of the new trust module
  • Feature flag tuf is now sigstore-trust-root
  • Struct ManualRepository is now ManualTrustRoot
  • Struct SigstoreRepository is now SigstoreTrustRoot
  • Trait Repository is now TrustRoot
  • Struct ManualTrustRoot, previously named ManualRepository, can now be used without dependencies that made it incompatible with wasm targets

Documentation

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

Xynnn007
Xynnn007 reviewed Feb 27, 2024
View reviewed changes
Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

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?

src/repository/mod.rs Outdated Show resolved Hide resolved
@tannaurus
Copy link
Contributor Author

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.

@tannaurus tannaurus changed the title Repository feature flag The Repository trait and ManualRepository no longer require a feature flag Feb 27, 2024
@tannaurus tannaurus changed the title The Repository trait and ManualRepository no longer require a feature flag The Repository trait and ManualRepository struct no longer require a feature flag Feb 27, 2024
@tnytown
Copy link
Contributor

tnytown commented Feb 27, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

tnytown commented Feb 28, 2024

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
Copy link
Member

Xynnn007 commented Mar 1, 2024

cc @flavio @viccuad

@flavio
Copy link
Member

flavio commented Mar 1, 2024

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
Copy link
Contributor Author

tannaurus commented Mar 1, 2024 •
edited
Loading

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:

  • 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

Thoughts? cc @tnytown @Xynnn007

@tnytown
Copy link
Contributor

tnytown commented Mar 1, 2024

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!

Xynnn007
Xynnn007 approved these changes Mar 2, 2024
View reviewed changes
Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

LGTM

@tannaurus
Copy link
Contributor Author

tannaurus commented Mar 2, 2024 •
edited
Loading

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
Copy link
Member

Xynnn007 commented Mar 7, 2024

@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
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

@tannaurus Yes. The lint error will be fixed by #334

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

flavio
flavio approved these changes Mar 11, 2024
View reviewed changes
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

Everything looks good to me. I think the warning messages can go away now that #334 has been merged. Try to rebase this PR against the main branch

@tannaurus
Moved the Repository trait and the ManualRepository struct out of 'tu…
717fb2d 
…f' feature flag: 'tuf' lacks support for wasm due to its dependencies.

Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Re-exported Repository trait and ManualRepository struct out of 'tuf'…
5588934 
… mod to preserve backwards compatibility.

Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Corrected previous commit: had previously re-exported from the wrong …
8db4b11 
…place.

Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Corrected copyright date: 2021 -> 2024
05be0b4 
Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Removed 'repository' feature flag: 'repository' modual is no longer h…
97357d3 
…idden behind a feature flag.

Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Renamed 'tuf' feature flag to 'sigstore-repository'
1b17986 
Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Revert "Renamed 'tuf' feature flag to 'sigstore-repository'"
7ab25d0 
This reverts commit 1e6a059.

Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Renamed 'repository' modual to 'repo'
7609a34 
Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Removed left over 'all' cfg attr, left behind from a revert.
0a72702 
Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Ran cargo fmt: set up Rust Rover recently and negelected to enable th…
2d1ceb9 
…at on save.

Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Renames: mod repo -> mod trust. trait Repository -> trait TrustRoot. …
92685e6 
…struct ManualRepository -> ManualTrustRoot.

Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Renames: mod tuf -> mod sigstore
78f680e 
Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Moved: sigstore module (previously tuf module) moved into sub-module …
54f236d 
…of 'trust'

Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Renamed impacted types in /examples
77ed1f9 
Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Renames: SigstoreRespository -> SigstoreTrustRoot
e7db276 
Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Renames: struct SigstoreRepository -> struct SigstoreTrustRoot. featu…
0ab5e61 
…re 'tuf' -> feature 'sigstore-trust-root'

Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Fixed up comments that still referenced previous iterations of the re…
01f0fb1 
…factor

Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Renamed SigstoreRootTrust to SigstoreTrustRoot
1f52b41 
Signed-off-by: Tanner Gill <contacttannergill@gmail.com>
@tannaurus
Copy link
Contributor Author

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

@Xynnn007 Xynnn007 merged commit d5ba303 into sigstore:main Mar 12, 2024
6 checks passed
flavio added a commit to flavio/sigstore-rs that referenced this pull request Mar 27, 2024
@flavio
Tag the 0.9.0 release
062ae79 
== What's Changed
* sign: init by @jleightcap in sigstore#310
* cargo audit: ignore RUSTSEC-2023-0071 by @jleightcap in sigstore#321
* chore(deps): Update json-syntax requirement from 0.9.6 to 0.10.0 by @dependabot in sigstore#319
* chore(deps): Update cached requirement from 0.46.0 to 0.47.0 by @dependabot in sigstore#323
* chore(deps): Update serial_test requirement from 2.0.0 to 3.0.0 by @dependabot in sigstore#322
* dep: update rustls-webpki, fold in pki_types by @jleightcap in sigstore#324
* chore(deps): Update cached requirement from 0.47.0 to 0.48.0 by @dependabot in sigstore#325
* chore(deps): Update json-syntax requirement from 0.10.0 to 0.11.1 by @dependabot in sigstore#327
* chore(deps): Update cached requirement from 0.48.0 to 0.49.2 by @dependabot in sigstore#329
* chore(deps): Update json-syntax requirement from 0.11.1 to 0.12.2 by @dependabot in sigstore#330
* lint: fix lint error of chrono and tokio by @Xynnn007 in sigstore#334
* chore(deps): Update base64 requirement from 0.21.0 to 0.22.0 by @dependabot in sigstore#332
* The `Repository` trait and `ManualRepository` struct no longer require a feature flag by @tannaurus in sigstore#331
* chore(deps): Bump actions/checkout from 4.1.1 to 4.1.2 by @dependabot in sigstore#336
* chore(deps): Update reqwest requirement from 0.11 to 0.12 by @dependabot in sigstore#341
* update tough dep by @astoycos in sigstore#340

== New Contributors
* @tannaurus made their first contribution in sigstore#331
* @astoycos made their first contribution in sigstore#340

**Full Changelog**: sigstore/sigstore-rs@v0.8.0...v0.9.0

Signed-off-by: Flavio Castelli <fcastelli@suse.com>
@flavio flavio mentioned this pull request Mar 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Xynnn007 Xynnn007 Xynnn007 approved these changes

@flavio flavio flavio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cosign client builder cannot build without tuf feature
4 participants
@tannaurus @tnytown @Xynnn007 @flavio