-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Added Workflow to verify ambassaor.json schema #758
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, thanks for your work on this.
I have one condition on approving and one suggestion for the future.
Condition: When merging this, please select the "squash and merge" option.
Suggestion: Test on your local machine before pushing. Pushing should not be your test. You do not need to push every commit you make =]
You can squash commits locally by using git interactive rebasing. Happy to share some additional resources if that would be helpful.
You can test GitHub actions locally using ACT.
"properties": { | ||
"type": { | ||
"type": "string", | ||
"enum": ["article", "talk", "video", "other"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have pending PRs which have more types than this.
@benjagm did you want to expand this list (and/or the list in the document)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets add:
- book
- paper
- initiative
- project
- working group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!! Very good job with this!!
I just left some comments with small suggestions.
@@ -0,0 +1,77 @@ | |||
{ | |||
"$schema": "http://json-schema.org/draft-07/schema#", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change the json schema version to use the current 2020-12?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjagm Sir , could please advice what should we do...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ramishj, The image you uploaded is not visible, could you try uploading again please?
} | ||
} | ||
}, | ||
"required": ["name", "img", "bio", "title", "github", "twitter", "linkedin", "company", "country", "contributions"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make twitter and linkedin optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments and suggestions.
Amazing job with this!!
Co-authored-by: Benjamin Granados <40007659+benjagm@users.noreply.github.com>
Updated schema version,added description to each field,expanded list of Type of contributions and made Linked and Twitter not mandatory
I have made the required changes you suggested . |
Thanks for you suggestion @Relequestual would keep in my mind in future while testing for workflows . |
fixed indentation error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow is failing after updating to the current draft 2020-12
. I think the dependency to use is other:
https://ajv.js.org/json-schema.html#draft-2020-12
Logs:
https://github.com/json-schema-org/community/actions/runs/9631882401
- name: Validate ambassadors.json | ||
run: | | ||
node -e " | ||
const Ajv = require('ajv'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To run the validation against a 2020-12 schema the dependency to use is: ajv/dist/2020
Right now the action is failing:
https://github.com/json-schema-org/community/actions/runs/9631882401
updated ajv import to support latest draft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job @ramishj ! This is a cool feature very much needed.
GitHub Issue: [#755 ]
Summary:
This pull request updates the GitHub Actions workflow to validate the
ambassadors.json
file on push and pull request events.Do you think resolving this issue might require an Architectural Decision Record (ADR)? (significant or noteworthy)
No