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

doc: remove Notary v2 reference #262

Merged
merged 20 commits into from
Aug 3, 2023
Merged

Conversation

yizha1
Copy link
Contributor

@yizha1 yizha1 commented Jun 9, 2023

Update:

  • remove Notary v2 reference for the following documents:
    • media/notary-e2e-scenarios.svg
    • requirements/key-revocation.md
    • requirements/requirements.md
    • requirements/scenarios.md
    • requirements/verification-by-reference.md
    • specs/plugin-extensibility.md
    • specs/signature-envelope-cose.md
    • specs/signature-envelope-jws.md
    • specs/signing-and-verification-workflow.md
    • specs/signing-scheme.md
    • specs/trust-store-trust-policy.md

6/11/2023: Added other PRs related to removing Notary v2 reference:

Signed-off-by: Yi Zha [email protected]

yizha1 added 3 commits June 9, 2023 22:08
Signed-off-by: Yi Zha <[email protected]>
Signed-off-by: Yi Zha <[email protected]>
@yizha1 yizha1 marked this pull request as ready for review June 9, 2023 14:40
requirements/requirements.md Outdated Show resolved Hide resolved
requirements/requirements.md Outdated Show resolved Hide resolved
requirements/scenarios.md Outdated Show resolved Hide resolved
requirements/scenarios.md Outdated Show resolved Hide resolved
requirements/scenarios.md Outdated Show resolved Hide resolved
requirements/scenarios.md Outdated Show resolved Hide resolved
specs/signature-envelope-jws.md Outdated Show resolved Hide resolved
specs/signature-envelope-jws.md Outdated Show resolved Hide resolved
specs/signing-and-verification-workflow.md Outdated Show resolved Hide resolved
specs/trust-store-trust-policy.md Outdated Show resolved Hide resolved
Copy link
Contributor

@SteveLasker SteveLasker left a comment

Choose a reason for hiding this comment

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

Thanks @yizha1 for the detailed changes.
When changing from Notary v2 to Notary Project, you'll need "The" preface. I've commented on all the changes I saw.

requirements/requirements.md Outdated Show resolved Hide resolved
requirements/requirements.md Show resolved Hide resolved
requirements/scenarios.md Outdated Show resolved Hide resolved
requirements/scenarios.md Show resolved Hide resolved
requirements/scenarios.md Show resolved Hide resolved
specs/trust-store-trust-policy.md Outdated Show resolved Hide resolved
specs/trust-store-trust-policy.md Outdated Show resolved Hide resolved
specs/trust-store-trust-policy.md Outdated Show resolved Hide resolved
specs/trust-store-trust-policy.md Outdated Show resolved Hide resolved
specs/trust-store-trust-policy.md Outdated Show resolved Hide resolved
@TheFoxAtWork
Copy link

I'm perplexed by this PR and have questions on the organizational structure, intent, and focus of the repos under the Notary Project organization. If I'm having this level of confusion contributors and adopters are likely to be experiencing the same.

Specifically:

  • The Notary Project organization contains a multitude of repositories, the ones I'm most concerned with currently are Notation, Notary, and now Notary Project.
  • It was my understanding that the effort formerly known as Notary v2 was in fact Notation - and reviewing the history of the Notation repo, it in fact began as such: notaryproject/notation@7e95a0b
  • The descriptors that have since been updated on the Notation repository previously reference a renaming of Notary v2 to Notation that articulate it as a "set of simple tooling".
  • Moving a CLI specific capability to its own repo is perfectly fine, however this repository now lacks clarity in its difference from Notary other than the identifier of "project" - a modifier that conflicts with the org name and further dilutes the distinct differences between the work within this repository and that of the Notary repository. Of interesting note is that the Notary repo is no longer pinned at the Org level but Notary Project is further contributing to misunderstandings of which does what.
  • The Notary repository details "The Notary project comprises a server and a client for running and interacting with trusted collections. " Whereas the proposed changes here detail: addressing and resolving gaps from the "legacy" Notary.. If this in indeed the case, and the repository is being used to develop the progression and modification of Notary to close these and to ensure compatibility for current adopters of Notary, that is unclear in the partial renaming and further conflates Notary, Notary project, and subsequently Notation.
  • The terminal use of "legacy" to describe Notary is particularly concerning as it implies Notary is intending to be archived or change functionality completely and break compatibility with its current incarnation.
  • The changes proposed here for renaming are incomplete as the term "Notary v2" is heavily saturated in the Notary Project repository - of particular concern is the README which would continue to perpetuate confusion.

I would like to see improved clarity on the difference between all three of these repositories within all three repositories, what problems they are attempting to address and how, and have this particular repository's (Notary Project) plans and milestones to evolve Notary (as is implied by its intent in this PR, but not by the historical record of its existing documentation) captured on a Roadmap/project board that is up-to-date and aligns with a similar roadmap/project board on the Notary repo reflecting the mirrored side of that roadmap if such replacement (as implied by this PR and the details of this repository) is the the goal of this Notary Project repo.

Copy link

@mattfarina mattfarina left a comment

Choose a reason for hiding this comment

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

I'm glad to see a PR that starts to clean up the language around v2 which has been a point of confusion for some. I have some general overall feedback to, hopefully, help you with this clarification effort.

  • While a number of files were touched in the change, I noticed the top level README.md still have the v2 language. This is the file people will see when they first approach the repo. Is there a reason for skipping that?
  • There are places where the grammar seems a little off. For example, "Notary Project specification implementation MUST enforce". I think the section is in reference to implementations of the specification. But, the grammar makes it difficult to read.
  • This bring me back to the different things which aren't completely clear. I think they are the following but it's not spelled out anywhere:
    • "legacy Notary"
    • The Notary Project Specification
    • Notation (which is a reference implementation of the specification)
  • While it's not spelled out, I think the signature is part of the specification.

Can I ask that the material is read as if it's an external party reading it. Like a new person to the project. I think reading it from that point of view might help clear up some of the grammar and confusion around assumptions.

requirements/requirements.md Outdated Show resolved Hide resolved
requirements/verification-by-reference.md Outdated Show resolved Hide resolved
requirements/scenarios.md Outdated Show resolved Hide resolved
requirements/scenarios.md Outdated Show resolved Hide resolved
@toddysm
Copy link
Contributor

toddysm commented Jun 9, 2023

Thank you, @TheFoxAtWork and @mattfarina for the feedback. Please bear with us as we go through these changes. I am reviewing the PR from @yizha1 and will push few commits over the weekend that remove those confusions and add clarity.

@yizha1
Copy link
Contributor Author

yizha1 commented Jun 10, 2023

Thanks @TheFoxAtWork @mattfarina for the comments. I will work with @toddysm to resolve comments and add clarify.

Answers to questons on readme file and signature spec:

While a number of files were touched in the change, I noticed the top level README.md still have the v2 language. This is the file people will see when they first approach the repo. Is there a reason for skipping that?

The readme file will be updated. I tried to keep one PR focusing on limited scope, which IMO helps to make reviewing more efficient. The readme.md file requires an overhaul. Not only removing Notary v2 reference, but also other changes like removing outdated commands or replacing those with a link to another doc.

While it's not spelled out, I think the signature is part of the specification

I also created another PR to address the signature specification, since the changes are not small, see #256

I should have added this context into the PR description earlier that the removing Notary v2 reference will include several PRs to complete it. More clarification will be added to this PR.

@yizha1 yizha1 requested review from SteveLasker and shizhMSFT June 10, 2023 07:09
@toddysm
Copy link
Contributor

toddysm commented Jun 11, 2023

@TheFoxAtWork and @mattfarina would you mind checking notaryproject/.github#32 and whether it addresses the concerns expressed above.

yizha1 added 4 commits July 21, 2023 19:43
Signed-off-by: Yi Zha <[email protected]>
Signed-off-by: Yi Zha <[email protected]>
Signed-off-by: Yi Zha <[email protected]>
Signed-off-by: Yi Zha <[email protected]>
@yizha1
Copy link
Contributor Author

yizha1 commented Jul 21, 2023

@mattfarina @TheFoxAtWork @toddysm @gokarnm @priteshbandi @SteveLasker
I updated this PR based on Proposal for bringing clarity to the Notary Project branding, please review it again.

@iamsamirzon I created a separate issue #269 to track your comments #262 (comment), since this PR mainly focused on naming issues.

Copy link

@TheFoxAtWork TheFoxAtWork left a comment

Choose a reason for hiding this comment

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

Thank you for the changes! I've left one comment regarding image v. artifact which is more a matter of consistency.

specs/signing-and-verification-workflow.md Show resolved Hide resolved
Signed-off-by: Yi Zha <[email protected]>
@yizha1 yizha1 requested a review from SteveLasker July 25, 2023 11:53
Signed-off-by: Yi Zha <[email protected]>
@yizha1 yizha1 requested a review from SteveLasker July 27, 2023 06:31
FeynmanZhou
FeynmanZhou previously approved these changes Jul 31, 2023
Copy link
Member

@FeynmanZhou FeynmanZhou left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @yizha1

requirements/requirements.md Outdated Show resolved Hide resolved
requirements/requirements.md Show resolved Hide resolved
specs/signature-envelope-cose.md Show resolved Hide resolved
Signed-off-by: Yi Zha <[email protected]>
specs/signing-and-verification-workflow.md Outdated Show resolved Hide resolved
specs/signing-scheme.md Outdated Show resolved Hide resolved
shizhMSFT
shizhMSFT previously approved these changes Aug 1, 2023
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Yi Zha <[email protected]>
Copy link
Contributor

@Two-Hearts Two-Hearts left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gokarnm gokarnm left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for these updates @yizha1 ! I added a question for value of config.mediaType here.

Copy link
Member

@FeynmanZhou FeynmanZhou left a comment

Choose a reason for hiding this comment

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

LGTM

@yizha1 yizha1 merged commit c259c8e into notaryproject:main Aug 3, 2023
yizha1 added a commit to notaryproject/.github that referenced this pull request Aug 7, 2023
@TheFoxAtWork and @mattfarina related to the
notaryproject/specifications#262 we would like to
have one central place where the overview is made and refer to it from
the other repositories. Please let me know if this PR answers your
question posted in
notaryproject/specifications#262.

---------

Signed-off-by: Toddy Mladenov <[email protected]>
Signed-off-by: Toddy Mladenov <[email protected]>
Co-authored-by: Emily Fox <[email protected]>
Co-authored-by: Milind Gokarn <[email protected]>
Co-authored-by: Yi Zha <[email protected]>
Co-authored-by: Samir Kakkar <[email protected]>
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.

Notation naming
10 participants