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

Maintenance: jmespath utility #1645

Closed
1 of 2 tasks
rjmackay opened this issue Aug 2, 2023 · 3 comments · Fixed by #2289
Closed
1 of 2 tasks

Maintenance: jmespath utility #1645

rjmackay opened this issue Aug 2, 2023 · 3 comments · Fixed by #2289
Assignees
Labels
completed This item is complete and has been merged/shipped dependencies Changes that touch dependencies, e.g. Dependabot, etc. idempotency This item relates to the Idempotency Utility

Comments

@rjmackay
Copy link

rjmackay commented Aug 2, 2023

Summary

The recently added idempotency feature adds a dependency on jmespath. While that package has many downloads, it hasn't been updated in 2 years.

This seems like a concern for the powertools tenets:

Keep it lean. Additional dependencies are carefully considered for security and ease of maintenance, and prevent negatively impacting startup time.

The key selection that jmespath is used for could easily be implemented without this dependency (see #1644)

Why is this needed?

jmespath seems unmaintained and not ideal for a production dependency.

Which area does this relate to?

JMESPath

Solution

  • Remove jmespath and replace with a much simpler method of selecting the idempotency key
  • Replace jmespath with a more well maintained implementation? (https://www.npmjs.com/package/@metrichor/jmespath is slightly more recent and in typescript)
  • Fork and own jmespath?

Acknowledgment

Future readers

Please react with 👍 and your use case to help us understand customer demand.

@rjmackay rjmackay added triage This item has not been triaged by a maintainer, please wait internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) labels Aug 2, 2023
@dreamorosi dreamorosi added dependencies Changes that touch dependencies, e.g. Dependabot, etc. idempotency This item relates to the Idempotency Utility discussing The issue needs to be discussed, elaborated, or refined and removed triage This item has not been triaged by a maintainer, please wait internal PRs that introduce changes in governance, tech debt and chores (linting setup, baseline, etc.) labels Aug 2, 2023
@dreamorosi dreamorosi self-assigned this Aug 2, 2023
@dreamorosi
Copy link
Contributor

Hi @rjmackay, thank you for opening the issue.

As stated in the tenet, we take the choice of dependencies seriously and have surveyed the different options when first implementing the Idempotency utility.

The jmespath library that we are currently using albeit not having been updated in a while, is maintained by the same people who maintain and govern the JMESPath specification. The module has no dependencies and the specification itself has remained fairly static, which I think are both indicators of why the library hasn't been updated recently.

When auditing the code for the library however, I have found that its implementation doesn't take advantage of modern language features and in general it's hard to reason about. Additionally, and more importantly for Powertools, it also doesn't support some JMESPath features like defining custom functions, a feature that we consider important for the Idempotency utility as well as other future utilities that handle request payloads.

We have indeed evaluated the other library you mentioned but ended up deciding to not use it for two reasons:

  • While the library has a more recent release than the original there are already signs that show it might not be a priority for the maintainers.
  • We felt more comfortable, as an interim solution, sticking to a library maintained by the same people who own the specification when it comes to major changes in JMESPath

With that said, we are still evaluating our options (including all the ones you mentioned) and are not yet set on using the jmespath library. The Idempotency utility is still in beta and we expect to have a clearer idea that we can share in the coming weeks and definitely before marking it ready for production.

Finally, regarding the value of using JMESPath in the first place versus a simple function, I would suggest we continue the discussion in the other issue you have opened where I have shared our rationale: #1644.

@dreamorosi dreamorosi added confirmed The scope is clear, ready for implementation and removed discussing The issue needs to be discussed, elaborated, or refined labels Feb 21, 2024
@dreamorosi dreamorosi changed the title Maintenance: Concern about dependency on jmespath Maintenance: jmespath utility Feb 21, 2024
@dreamorosi dreamorosi linked a pull request Mar 6, 2024 that will close this issue
9 tasks
@dreamorosi dreamorosi removed a link to a pull request Mar 27, 2024
9 tasks
@dreamorosi dreamorosi linked a pull request Mar 27, 2024 that will close this issue
9 tasks
Copy link
Contributor

github-actions bot commented Apr 2, 2024

⚠️ COMMENT VISIBILITY WARNING ⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

@github-actions github-actions bot added pending-release This item has been merged and will be released soon and removed confirmed The scope is clear, ready for implementation labels Apr 2, 2024
Copy link
Contributor

This is now released under v2.0.4 version!

@github-actions github-actions bot added completed This item is complete and has been merged/shipped and removed pending-release This item has been merged and will be released soon labels Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
completed This item is complete and has been merged/shipped dependencies Changes that touch dependencies, e.g. Dependabot, etc. idempotency This item relates to the Idempotency Utility
Projects
Development

Successfully merging a pull request may close this issue.

2 participants