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

add schemas for metadata and config #370

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

add schemas for metadata and config #370

wants to merge 3 commits into from

Conversation

sed-i
Copy link

@sed-i sed-i commented May 27, 2021

Addresses #369.

Having json schemas would be very helpful for reducing runtime errors:

  1. Auto-completion + validation from within the IDE
  2. Auto-validation as part of charmcraft build

@jujubot
Copy link
Contributor

jujubot commented May 27, 2021

Can one of the admins verify this patch?

1 similar comment
@jujubot
Copy link
Contributor

jujubot commented May 27, 2021

Can one of the admins verify this patch?

cjp256
cjp256 reviewed May 28, 2021
View reviewed changes
Copy link

@cjp256 cjp256 left a comment

Choose a reason for hiding this comment

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

Nicely done! While I'm not knowledgeable enough to review the accuracy of the schema, I like how you organized this.

I only have some nitpicks:

  • there are some inconsistencies in the formatting (extra lines in places and not others, field ordering etc.) - perhaps run it through a formatter, or use python's json.dumps(..., sort_keys=True) to quickly do this?
  • consider addressing the mixed usage of capitalization, periods, etc. in descriptions to be consistent.

schemas/metadata-schema.json Outdated Show resolved Hide resolved
schemas/metadata-schema.json Outdated Show resolved Hide resolved
schemas/metadata-schema.json Outdated Show resolved Hide resolved
schemas/metadata-schema.json Outdated Show resolved Hide resolved
schemas/metadata-schema.json Outdated Show resolved Hide resolved
sed-i and others added 2 commits May 28, 2021 11:11
@sed-i @cjp256
fix phrasing
9bcc560 
apply @cjp256 suggestions from code review

Co-authored-by: Chris Patterson <[email protected]>
@sed-i
remove empty lines
be32c13
@sed-i sed-i marked this pull request as ready for review June 10, 2021 15:55
johnsca
johnsca approved these changes Jun 10, 2021
View reviewed changes
Copy link

@johnsca johnsca left a comment

Choose a reason for hiding this comment

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

Nice, it will be good to have this as a reference to point to and to be able to build upon.

@sed-i
Copy link
Author

sed-i commented Jun 10, 2021

@hpidcock, @jameinel could you please take a look?

@SimonRichardson
Copy link
Member

I'll bring this one up, to see if there are any issues in landing this one.

@sed-i sed-i mentioned this pull request Apr 3, 2023
Merged
@sed-i sed-i mentioned this pull request Sep 16, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjp256 cjp256 cjp256 left review comments

@johnsca johnsca johnsca approved these changes

At least 1 approving review is required to merge this pull request.

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

Successfully merging this pull request may close these issues.
5 participants
@sed-i @jujubot @SimonRichardson @johnsca @cjp256