-
Notifications
You must be signed in to change notification settings - Fork 26
fix: replace broken validate.sh by validate.js
#285
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
Conversation
12eae26 to
9920267
Compare
afrittoli
left a comment
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.
Thanks @davidB - the shell script was a bit hacky so it's good to have a better implementation.
The only caveat on my side is that I don't know JS so I have a hard time reviewing this and I wouldn't be maintaining the script in future.
There is another effort ongoing to move the release scripts to Python, perhaps we could try and converge to that language instead?
I believe the reason I used the JS tool for this is that it was the best implementation I could find of a tool to validate against JSON schemas.
If @xibz is happy with JS it works for me - only as I said I will have a harder time helping on tools for this repo.
BASH script has several issues: 1. Doesn't failed on error, because in bash variable (like `exampled_failed`) updated into a while loop are not updated outside the loop, because a while loop is a subprocess, you can look at https://www.baeldung.com/linux/while-loop-variable-scope 2. find: warning: you have specified the global option -maxdepth after the argument -type, but global options are not positional, i.e., -maxdepth affects tests specified before it as well as those specified after it. Please specify global options before other arguments. Because 1. is a symptom of the limit of bash for this script, and the tools already require the javascript stack (nodejs/bun/...) I replace the validate.sh by validate.js Signed-off-by: David Bernard <david.bernard.31@gmail.com>
…ext.specversion` Signed-off-by: David Bernard <david.bernard.31@gmail.com>
f8afd8c to
fd2d389
Compare
|
@afrittoli , I also comment on #276 , I'm in favor to not multiple tool "stack" (python, go, javascript) as build tool. TBH, I translated the bash script to js with AI, then I tuned, fixed & integrated it in the CI. As mentioned I selected javascript because it was already used (not python). I don't have strong preferences between the "scripting" languages (they have pros & cons), but in every case we'll need to use libraries (their std lib will not be enough) and maybe called some cli, so to setup the stack... |
afrittoli
left a comment
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.
Thanks!
/approve
Changes
validate.shbyvalidate.jsthat failed the CI on invalid json schema or conformance *.jsonBASH script has several issues:
exampled_failed) updated into a while loop are not updated outside the loop, because a while loop is a subprocess, you can look at https://www.baeldung.com/linux/while-loop-variable-scope soexampled_failedare not used to detect invalid conformance vs json schemaBecause 1. is a symptom of the limit of bash for this script, and the tools already require the javascript stack (nodejs/bun/...) I replace the validate.sh by validate.js with the fix (see CI that failed)
FIX #284
Submitter Checklist
As the author of this PR, please check off the items in this checklist: