-
Notifications
You must be signed in to change notification settings - Fork 129
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
Add option to enable JSON output for diff/sync #798
Conversation
Hi @DanielRailean , thanks for your contribution! I gave this a first review. Other than the comments I left, can you please make sure to address the linting errors? |
d898d63
to
a892bf3
Compare
d1c561b
to
d225b21
Compare
Hi @GGabriele, please have a second look when you got a moment, and let me know if other changes are needed. |
Hey there, @GGabriele please have a second look at this. It says change requested, but this seems to be a GitHub bug. |
0ebbd1a
to
e13e34c
Compare
@GGabriele updates here? |
@hbagdi @GGabriele pinging you guys again |
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.
@DanielRailean apologies for the delay in coming back to you
I've added some comments and asks for changes. On top of those, I kindly ask you to please:
- make sure your branch is in sync with the
main
branch - make sure you add some unit tests (I pointed out where this is needed in the code)
- add some simple integration tests as well (you can find some example here)
- make sure lint tests pass
Thanks for your patience!
be58614
to
ec67374
Compare
@DanielRailean nice improvements! Feel free to "pin" the diff integration tests for post-3.x versions to avoid failures due to schema changes. For example:
|
Co-authored-by: Gabriele <gabrielegerbino@gmail.com>
Co-authored-by: Gabriele <gabrielegerbino@gmail.com>
Co-authored-by: Gabriele <gabrielegerbino@gmail.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #798 +/- ##
==========================================
- Coverage 34.03% 33.70% -0.33%
==========================================
Files 100 100
Lines 11884 12000 +116
==========================================
Hits 4045 4045
- Misses 7446 7562 +116
Partials 393 393
☔ View full report in Codecov by Sentry. |
990ac49
to
66698e6
Compare
was trying to test the v3.0.0 but if you're ok with it, I'm giving up on them. |
Feel free to give up on them. The easy alternative would be to create I'm okay either ways. |
Thanks for the approval :) |
Do you want to manually squash it and add some description in your commit? Otherwise I'll squash myself via GH and merge it right away. This should be released later today :) Thanks for your great work and apologies once more for the looong delay. |
I don't think I have the permissions to squash. (or maybe it's because of some of the failed actions), so it's better if you do it 😃 Thank you too for the help and patience. 🚀 |
We’ve been having an issue with
deck
where we wanted to parse its output to something meaningful like an object so we think that it would be useful to have an option that would allow its output to be a JSON object that can be then used with pretty much any language to build SDKs/clients.This PR does exactly that, enabling the above-mentioned feature using the
enable-json-output
flag.You can check out a small demo below: