-
Notifications
You must be signed in to change notification settings - Fork 63
Add JSON5 parsing capabilities for integration configs #1732
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
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.
Hi @devesh-2002, thank you for taking this on! In generally this is looking good to me, however, could you also sign off for your commit?
git commit -s -m ""
Is this okay? |
From the DCO action details, signing off has 3 steps:
As far as the PR goes: This looks good at a glance, thanks for contributing! You might also consider adding some unit tests (or modifying existing ones) to cover the new functionality. I'll be available later this week to get this merged/backported. |
Signed-off-by: YANGDB <[email protected]> Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: [email protected] <[email protected]>
Hello. I have signed off the commits and I have also tried to add some tests. Could you please check it? @Swiddis |
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 for adding DCO and tests. I enabled the CI workflows for the PR to help narrow the feedback loop, but I think there's still some more changes to look at. Thanks again for contributing!
server/adaptors/integrations/__data__/repository/aws_vpc_flow/assets/vpc_live_all_mv-1.0.0.sql
Outdated
Show resolved
Hide resolved
server/adaptors/integrations/__test__/local_fs_repository.test.ts
Outdated
Show resolved
Hide resolved
Signed-off-by: [email protected] <[email protected]>
This reverts commit 71597f5. Signed-off-by: [email protected] <[email protected]>
Hello,@Swiddis. Can you check if it is correct now? |
You can run tests locally with
In particular the tests in
In this case it's because the file extension was changed to json5, but there's no handling for json5 filenames. You can try to implement this handling in the code if you'd like, or you can change the extensions back to json but keep the json5-formatted data here. |
Signed-off-by: [email protected] <[email protected]>
Signed-off-by: Devesh Rahatekar <[email protected]>
I have changed the extensions to back to JSON keeping the data as JSON5. |
Signed-off-by: Simeon Widdis <[email protected]>
Looks like it's mostly working now, just some remaining unit tests that need to be updated for the new error messages: https://github.com/opensearch-project/dashboards-observability/actions/runs/8899395226/job/24438654843?pr=1732#step:6:10907 Again, I recommend running tests locally instead of waiting on actions here, it makes for a faster feedback loop and the tests are typically good at catching these types of small compatibility issues. |
Signed-off-by: [email protected] <[email protected]>
I tried to change the
|
Confirmed that tests are passing after fixing upstream breakage -- will merge this after #1792. Thanks again for the contribution! |
Thanks a lot for all the suggestions in this PR! @Swiddis @RyanL1997 |
* update live mv table name (#1725) Signed-off-by: YANGDB <[email protected]> Signed-off-by: [email protected] <[email protected]> * Integrated JSON5 Signed-off-by: [email protected] <[email protected]> * json_data_adaptor Signed-off-by: [email protected] <[email protected]> * Added tests for JSON5 Signed-off-by: [email protected] <[email protected]> * Updated integration configs to json5 Signed-off-by: [email protected] <[email protected]> * Revert "update live mv table name (#1725)" This reverts commit 71597f5. Signed-off-by: [email protected] <[email protected]> * Updated filename Signed-off-by: [email protected] <[email protected]> * Delete t --count 5c93001..fe64d02 Signed-off-by: Devesh Rahatekar <[email protected]> * Updated some integrations Signed-off-by: [email protected] <[email protected]> --------- Signed-off-by: YANGDB <[email protected]> Signed-off-by: [email protected] <[email protected]> Signed-off-by: Devesh Rahatekar <[email protected]> Signed-off-by: Simeon Widdis <[email protected]> Co-authored-by: YANGDB <[email protected]> Co-authored-by: Simeon Widdis <[email protected]> (cherry picked from commit 9532b7d) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Integrated JSON5
Issues Resolved
Issue #1680
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.