-
-
Notifications
You must be signed in to change notification settings - Fork 269
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
ci: add a script and CI job to validate spec examples #1046
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Exclude them, yes.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is gonna be pretty slow. Can we run it async somehow and compare performance? 🤔
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied suggestion through baa7c12
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.
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toasyncapi
command (CLI) throughxargs
would 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice oneliner 👏 👏
There is only a couple of issues here:
- The path to skip should be
'examples/social-media/common
. - 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'
- With
-exec
option you can't parallelise the work, so everything will run sequentially.
Instead, try piping the find output toxargs
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
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.
Additionally, please add a comment on top of the command explaining why we are skipping that dir.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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 changed PR prefix cause otherwise we would release new spec version 😁
also, one question, if we use AsyncAPI CLI on each PR - what if we do a PR that adds new features for new release, and updates examples 🤔
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
omg, bash on steroids
luckily we have chat gpt to translate it for me when needed 😁
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.
omg, bash on steroids
luckily we have chat gpt to translate it for me when needed 😁
😆
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so should be use a specific version of the CLI? 👀
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @derberg @AnimeshKumar923
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.
Ping @derberg 👋
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I suggest then we can try with this but opened to change if needed at any time.
@asyncapi/bounty_team |
UPDATE
|
Quality Gate passedIssues Measures |
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.
LGTM!
/rtm |
This PR adds a script and CI to validate the AsyncAPI documents in the
/examples
directory.Tasks
/examples
directory.Will be done through a separate PR (context) (phase-2)Being done through--> achieved through ci: script to validate embedded examples in the asyncapi.md file #1059cc: @smoya PTAL
Related issue(s):
#957 (partially resolves)