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

Schema cleanup #798

Merged
merged 2 commits into from
Jun 12, 2023
Merged

Schema cleanup #798

merged 2 commits into from
Jun 12, 2023

Conversation

nissimsan
Copy link
Collaborator

This removes a bunch of schema elements which are not used:

  • name
  • description
  • relatedLinks

Copy link
Collaborator

@mkhraisha mkhraisha left a comment

Choose a reason for hiding this comment

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

To note, name and description will still be valid fields im those credentials as they are part of the verifiable credential spec.

@nissimsan
Copy link
Collaborator Author

Awaiting Mesur to add an approval on this. Then agreement on the call to merge out of band.

@OR13 OR13 mentioned this pull request Jun 6, 2023
@brownoxford
Copy link
Collaborator

@rhofvendahl please take point on this research

@rhofvendahl
Copy link
Collaborator

@nissimsan There are a number of ag related schemas which include include name, description, and relatedLink as fields, but in most cases only name is used in the example.

@mkhraisha you're saying that it'd be fine to leave these name values in the example even if we remove that yaml field? & this holds true even with additionalProperties set to false?

I'm not sure I understand the pattern we're following if that's the case. Are we only including yaml fields that are both in the spec and in use in examples?

@nissimsan
Copy link
Collaborator Author

nissimsan commented Jun 7, 2023

@rhofvendahl, that should not be the case. Do you mean issuer's name? That's something else.

If there are indeed a few credential name I've missed, please point me to which ones they are and I'll clean it out. (edit: CI would have missed them too)

@rhofvendahl
Copy link
Collaborator

rhofvendahl commented Jun 8, 2023

@nissimsan If I'm understanding the intent here it looks as though most ag-related schemas plus a few others have the name & description properties, including:
CTPATCertificate
DeliveryScheduleCredential
DeliveryStatementCredential
FoodDefenseInspectionCredential
FoodGradeInspectionCredential
FSMACreatingCTECredential
FSMAFirstReceiverDataCredential
FSMAGrowingCTECredential
...

(might make more sense to just search for it actually)

For example, here's an excerpt from what I'm seeing for FSMAReceivingCTECredential.yml when I check out this branch:

  id:
    type: string
  name:
    type: string
  description:
    type: string
  issuanceDate:
    type: string
  expirationDate:
    type: string
  issuer:
    $ref: ../common/Organization.yml

Like I said a number of these do have values for name & relatedLink in the example - I'm not sure how to handle that

Please let me know if I'm misunderstanding something!

@nissimsan
Copy link
Collaborator Author

@rhofvendahl, correct.

All the latter ones are because I didn't want to touch the Oil+Gas and Agri schemas. I definitely encourage that you have similar considerations on "your" schemas and purge unused fields.

The CTPAT still has name and descr because it makes business sense for it to include them, providing holders and verifiers a human readable context of the certificate.

@rhofvendahl
Copy link
Collaborator

@nissimsan Ah, that makes sense. No objections then.

@nissimsan nissimsan merged commit fb33ec5 into main Jun 12, 2023
2 checks passed
@nissimsan nissimsan deleted the name-descr-relatedlinks-schema-cleanup branch August 8, 2023 18:42
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.

None yet

5 participants