Skip to content
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

Use more up-to-date and comprehensive JSONPath library #486

Merged
merged 3 commits into from
Aug 23, 2023

Conversation

b-erdem
Copy link
Contributor

@b-erdem b-erdem commented Aug 23, 2023

Recently we noticed that Argo Workflow doesn't support some JSONPath expression to extract some values from inputs. After digging a bit we found that current JSONPath library used by Argo is outdated and it does support only a fraction of the spec. After looking around for better JSONPath libraries I found this one that works for the all expressions I have tried so far: github.com/evilmonkeyinc/jsonpath

If you're also curious about other libraries you can see a comparison here but keep in mind that it's either outdated or incorrect: https://asaiyusuke.github.io/jsonpath/cburgmer-json-path-comparison/docs/index.html

Because I tried some of the JSONPath libraries listed there, including the one written by the author of the post above and most of the expressions in the comparison table didn't work.

  • As I see these changes don't cause any issue or break existing functionality.

  • I have added some test cases to cover more complex expressions.

Please let me know if there's any mistake or something I'm missing when opening a PR here.

@b-erdem
Copy link
Contributor Author

b-erdem commented Aug 23, 2023

@agilgur5 Hello Anton, I opened this PR to change the JSONPath library.

Baris Erdem added 2 commits August 23, 2023 11:04
Signed-off-by: Baris Erdem <baris.erdem@phrase.com>
Signed-off-by: Baris Erdem <baris.erdem@phrase.com>
@agilgur5
Copy link
Member

For posterity, this is a follow-up from Slack

Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for adding tests too!

@agilgur5
Copy link
Member

I looked through some of the Go JSONPath libraries as well and noticed that several were unmaintained (and incomplete) unfortunately 😕
The new library in this PR is not that popular, but none of them really were as far as I could tell. This one does have some decent docs though

@agilgur5 agilgur5 added dependencies Pull requests that update a dependency file go Pull requests that update Go code labels Aug 23, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 23, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@terrytangyuan terrytangyuan enabled auto-merge (squash) August 23, 2023 17:07
@terrytangyuan terrytangyuan merged commit a672276 into argoproj:master Aug 23, 2023
6 checks passed
@agilgur5
Copy link
Member

@b-erdem could you make a PR to Workflows to update this dep now?

@b-erdem
Copy link
Contributor Author

b-erdem commented Aug 23, 2023

@b-erdem could you make a PR to Workflows to update this dep now?

Hi, thanks for merging it! I can open a PR on Workflows in a few hours once I’m at home. Does that work?

@agilgur5
Copy link
Member

Yea no rush or anything

@agilgur5
Copy link
Member

@b-erdem just a friendly reminder that the full update still requires a PR to Workflows

@b-erdem
Copy link
Contributor Author

b-erdem commented Aug 29, 2023

@agilgur5 Thanks for the reminder. I’m working on it. It turned out I needed to make a few changes on the main repo. Just updating the dependencies was not enough. I’ll just open a PR and we can discuss the changes I made there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file go Pull requests that update Go code
Projects
None yet
3 participants