ci: add a script and CI job to validate spec examples #1046
Conversation
Changes: - added initial version of the workflow file - will be worked and moved forward with this file with the required modifications
Changes: - updated the checkout action version - change the main branch to master
Changes: - added the both YAML file extension to validate all files present in the examples directory and further sub-directories
Changes: - updated code to validate the example folder and sub-folders - add dev dependecies for the script
Changes: - removed the 'test' script - improved the 'validate-examples' workflow file code
Changes: - migrated package.json to root of the directory - changes package.json accordingly
Changes: - update script accordingly to the newly migrated package.json file - update workflow yaml with workflow_dispatch events
AnimeshKumar923
requested review from
fmvilas,
derberg,
dalelane,
smoya,
char0n,
asyncapi-bot-eve and
GreenRover
as code owners
| const glob = require('glob'); | ||
|
|
||
| // Use glob to find all YAML files in the examples directory | ||
| const files = glob.sync('./examples/**/*.{yml,yaml}'); |
There was a problem hiding this comment.
I believe there are some files you won't be able to validate since they are not AsyncAPI documents such as the files located in https://github.com/asyncapi/spec/tree/master/examples/social-media/common. Not sure if there are more, you will need to check.
There was a problem hiding this comment.
Yes, it is like that.
You can see the output:
So, what can be done about it? Exclude these? Or something else?
There was a problem hiding this comment.
Exclude them, yes.
Got it!
keeping this conversation unresolved for the time being, to serve as a reminder
| filesCount++; | ||
| try { | ||
| console.log(`\nValidating: ${file}`); | ||
| execSync(`npx asyncapi validate ${file}`, { stdio: 'inherit' }); |
There was a problem hiding this comment.
This is gonna be pretty slow. Can we run it async somehow and compare performance? 🤔
There was a problem hiding this comment.
This is gonna be pretty slow. Can we run it async somehow and compare performance? 🤔
Okay, let me try doing it asynchronously...
There was a problem hiding this comment.
Applied suggestion through baa7c12
There was a problem hiding this comment.
Performance Report
Before
When the script was executed synchronously:
- Job link
- Screenshot
After
When the script was executed asynchronously:
- Job link
- Screenshot
Conclusion: The execution time here has been reduced by 1m 10s with the use of asynchronous way of JavsScript.
Thanks for the suggestion! @smoya 👍
There was a problem hiding this comment.
Execution time for the script itself
Before
After
There was a problem hiding this comment.
I think we can go further and skip using a JS script here and find a simple online terminal command that do the job.
I believe by using find which also allows excluding paths + piping to asyncapi command (CLI) through xargs would end up being a really simple oneliner, not complicated and simple to read and maintain.
There was a problem hiding this comment.
I think we can go further and skip using a JS script here and find a simple online terminal command that do the job.
I believe by using
findwhich also allows excluding paths + piping toasyncapicommand (CLI) throughxargswould end up being a really simple oneliner, not complicated and simple to read and maintain.
Let me look into that...
There was a problem hiding this comment.
@smoya The code has been successfully executed. Please have a look at this workflow job detail
Attaching screenshot for more details:
Job run details
Changes: - applied suggestion from: https://github.com/asyncapi/spec/pull/1046/files?diff=unified&w=0#r1535456030
Changes: - removed the mistakenly placed code block from the line
Changes: - made the script asynchronous in nature as suggested by Sergio to make it's performance more better - applied suggestion from here: https://github.com/asyncapi/spec/pull/1046/files#r1535471235
Changes: - tried using the github-action-for-cli, but the approach failed. - going to use this approach by installing the asyncapi cli and proceed with it accordingly
| run: npm install -g @asyncapi/cli | ||
| - name: Validate AsyncAPI documents | ||
| run: | | ||
| find examples/ \( -path 'examples/social-media/*' -prune \) -o -type f \( -name "*.yml" -o -name "*.yaml" \) -exec asyncapi validate {} \; |
There was a problem hiding this comment.
Nice oneliner 👏 👏
There is only a couple of issues here:
- The path to skip should be
'examples/social-media/common. - I don't find
pruneoption to be very readable. Instead, you can use the-notoperator. E.g.-not -path 'examples/social-media/common' - With
-execoption you can't parallelise the work, so everything will run sequentially.
Instead, try piping the find output toxargsand the-Parg.
Applying that, will look like:
find examples -type f \( -name "*.yml" -o -name "*.yaml" \) -not -path 'examples/social-media/common' | xargs -P 10 -L 1 asyncapi validateThere was a problem hiding this comment.
Additionally, please add a comment on top of the command explaining why we are skipping that dir.
There was a problem hiding this comment.
Nice oneliner 👏 👏
Thank you! 😁 To be honest, I'm still learning bash so somethings may take time to understand 😅 and many things are new like the usage of xargs. Will learn along the way, thanks to you... 😊
There is only a couple of issues here:
1. The path to skip should be `'examples/social-media/common`. 2. I don't find `prune` option to be very readable. Instead, you can use the `-not` operator. E.g. `-not -path 'examples/social-media/common'` 3. With `-exec` option you can't parallelise the work, so everything will run sequentially. Instead, try piping the find output to `xargs` and the `-P` arg.Applying that, will look like:
find examples -type f \( -name "*.yml" -o -name "*.yaml" \) -not -path 'examples/social-media/common' | xargs -P 10 -L 1 asyncapi validate
Thank you for the suggestions, will try to understand and implement the same. 🙏
There was a problem hiding this comment.
Additionally, please add a comment on top of the command explaining why we are skipping that dir.
Sure, I'll do that.
There was a problem hiding this comment.
applied through cb7d070
Changes: - applied Sergio's suggestion from: https://github.com/asyncapi/spec/pull/1046/files#r1555684920 - added comment as to why we're skipping the mentioned directory
There was a problem hiding this comment.
On my side, this has my 👍 (pending to switch the branch from script-ci-spec-validation to master in the workflow).
Additinoally, if we want to merge this PR before continuing the work for Write a script to validate the AsyncAPI documents containing embedded examples. which I believe we should, please @AnimeshKumar923 remove the sentence Fixes https://github.com/asyncapi/spec/issues/957 from the description so the issue remains open after merging.
Can you @derberg do a quick check on the workflow just in case I'm missing something critical? 🙏
Yes, I'll remove the |
derberg
changed the title
| name: Validate AsyncAPI Examples | ||
|
|
||
| on: | ||
| push: |
There was a problem hiding this comment.
remember to remove
Yes.
| run: npm install -g @asyncapi/cli | ||
| - name: Validate AsyncAPI documents | ||
| run: | | ||
| find examples -type f \( -name "*.yml" -o -name "*.yaml" \) -not -path 'examples/social-media/common/*' | xargs -P 10 -L 1 asyncapi validate |
There was a problem hiding this comment.
omg, bash on steroids
luckily we have chat gpt to translate it for me when needed 😁
There was a problem hiding this comment.
omg, bash on steroids
luckily we have chat gpt to translate it for me when needed 😁
😆
There was a problem hiding this comment.
Come on! are self explanatory 😆 Let me save your time
find examples -type f \( -name "*.yml" -o -name "*.yaml" \) -not -path 'examples/social-media/common/*' This is completely human readable: find in examples dir "things" of type file (f) with name suffixes yml or yaml and do not include the path 'examples/social-media/common/*' . The output is a list of paths to those files.
xargs -P 10 -L 1 asyncapi validate Here maybe the trickiest part: xargs meaning do "x" with the piped arguments (which is the list of files), the -L 1 means take each new line as one execution, -P 10 means parallellize in 10 runs at the same time, and for each of those arguments (paths), execute asyncapi validate command.
| with: | ||
| node-version: '20' | ||
| - name: Install AsyncAPI CLI | ||
| run: npm install -g @asyncapi/cli |
There was a problem hiding this comment.
bo it uses latest CLI - advantage is that all latest features will be used but obvious disadvantage is randomly failing CI cause of some breaking change in validation, or some new validation enabled.
yeah, not so obvious in this case 😕
There was a problem hiding this comment.
so should be use a specific version of the CLI? 👀
There was a problem hiding this comment.
If I can give my opinion here, I think using latest is a good option. We are always testing on the last version, supporting last spec schemas, etc + if something is wrong with the CLI, we are gonna be one of the first who notice.
There was a problem hiding this comment.
no strong opinion really, there are pros and cons for using latest and not using latest. We just need to be aware that if we decide to use latest - then on PRs we can have random fails from time to time and we need to support contributors
There was a problem hiding this comment.
Agree. I suggest then we can try with this but opened to change if needed at any time.
|
@asyncapi/bounty_team |
UPDATE
|
|
|
/rtm |
This PR adds a script and CI to validate the AsyncAPI documents in the
/examplesdirectory.Tasks
/examplesdirectory.cc: @smoya PTAL
Related issue(s):
#957 (partially resolves)