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

add json_patch, json_patch_recipe and json_diff filters #9565

Merged
merged 18 commits into from
Jan 21, 2025

Conversation

numo68
Copy link
Contributor

@numo68 numo68 commented Jan 12, 2025

SUMMARY

The Ansible collections lack a good way to modify JSON data. This PR leverages the jsonpatch package and adds three filters implementing RFC 6902 operations on an object.

ISSUE TYPE
  • New Module/Plugin Pull Request
COMPONENT NAME

JavaScript Object Notation (JSON) Patch Filter

ADDITIONAL INFORMATION

There is a previous work done in the (https://github.com/ParticleDecay/ansible-jsonpatch) repo, but I think that the operations themselves should be available on arbitrary data, not just on files; a file modification can be added as a role if necessary.

I am a moderate Ansible user, but the first time contributor to Ansible collections, so if I forgot something, please kindly point out the deficiencies and I'll do my best to fix it.

Thanks for a great tool and great modules!

@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit labels Jan 12, 2025
@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Jan 12, 2025
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Thanks for your contriubtion! I've added some first comments.

Can you also add integration tests (tests/integration/targets/filter_json_patch/; see for example tests/integration/targets/filter_jc/ for how to install the required library before running the tests)?

plugins/filter/json_diff.yml Outdated Show resolved Hide resolved
plugins/filter/json_diff.yml Outdated Show resolved Hide resolved
plugins/filter/json_patch.py Outdated Show resolved Hide resolved
plugins/filter/json_patch.py Outdated Show resolved Hide resolved
plugins/filter/json_patch.py Outdated Show resolved Hide resolved
tests/unit/plugins/filter/test_json_diff.py Outdated Show resolved Hide resolved
@felixfontein
Copy link
Collaborator

@ansibullbot

This comment was marked as outdated.

@ansibullbot

This comment was marked as outdated.

plugins/filter/json_diff.yml Outdated Show resolved Hide resolved
tests/unit/plugins/filter/test_json_diff.py Outdated Show resolved Hide resolved
plugins/filter/json_patch.py Show resolved Hide resolved
plugins/filter/json_diff.yml Show resolved Hide resolved
plugins/filter/json_patch.py Outdated Show resolved Hide resolved
@numo68
Copy link
Contributor Author

numo68 commented Jan 12, 2025

  • comments resolved
  • input validation added
  • integration test added

@ansibullbot ansibullbot added integration tests/integration and removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Jan 12, 2025
plugins/filter/json_patch.yml Outdated Show resolved Hide resolved
plugins/filter/json_patch.yml Outdated Show resolved Hide resolved
plugins/filter/json_patch.yml Outdated Show resolved Hide resolved
plugins/filter/json_patch.yml Outdated Show resolved Hide resolved
plugins/filter/json_patch.yml Outdated Show resolved Hide resolved
plugins/filter/json_patch.py Outdated Show resolved Hide resolved
plugins/filter/json_patch.yml Show resolved Hide resolved
tests/integration/targets/filter_json_patch/tasks/main.yml Outdated Show resolved Hide resolved
tests/integration/targets/filter_json_patch/tasks/main.yml Outdated Show resolved Hide resolved
tests/unit/plugins/filter/test_json_patch.py Outdated Show resolved Hide resolved
@numo68
Copy link
Contributor Author

numo68 commented Jan 13, 2025

Many thanks for the valuabe input. I have addressed the - hopefully all - comments. The filter now accepts binary data (even if not possible to pass it from the play) and there is a fail_test argument to specify whether a failed test returns none or fails. The default is still none as it will be IMO more used to take decisions than to check validity.

Thanks

Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

Hi @numo68 Thanks for contributing!

Got a couple of comments.

plugins/filter/json_patch.yml Show resolved Hide resolved
plugins/filter/json_patch.py Show resolved Hide resolved
plugins/filter/json_patch.py Show resolved Hide resolved
plugins/filter/json_patch.yml Show resolved Hide resolved
plugins/filter/json_patch.yml Show resolved Hide resolved
tests/unit/plugins/filter/test_json_patch.py Show resolved Hide resolved
tests/unit/plugins/filter/test_json_patch.py Show resolved Hide resolved
tests/unit/requirements.txt Show resolved Hide resolved
plugins/filter/json_patch.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@russoz russoz left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Looks good to me. If nobody objects, I'll merge this in a few days.

plugins/filter/json_patch.py Outdated Show resolved Hide resolved
@felixfontein felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Jan 21, 2025
@felixfontein felixfontein merged commit f5c1b9c into ansible-collections:main Jan 21, 2025
138 checks passed
Copy link

patchback bot commented Jan 21, 2025

Backport to stable-10: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-10/f5c1b9c70f4c50dc840a3335c2bb2de000382e05/pr-9565

Backported as #9597

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Jan 21, 2025
* add json_patch, json_patch_recipe and json_diff filters

* fix copyright notices

* fix documentation

* fix docs, add maintainer

* fix review remarks

* add integration test

* fix docs (positional)

* add input validation

* formatting fixes

* more typing tweaks

* documentation fix

* fix review comments

* simplicfy input checking

* accept bytes and bytearray input

* add the fail_test argument

* fix docs format

* fix typing hints

* remove unneeded __future__ imports

(cherry picked from commit f5c1b9c)
@felixfontein
Copy link
Collaborator

@numo68 thanks for your contribution!
@russoz @bcoca thanks for reviewing!

felixfontein pushed a commit that referenced this pull request Jan 21, 2025
…cipe and json_diff filters (#9597)

add json_patch, json_patch_recipe and json_diff filters (#9565)

* add json_patch, json_patch_recipe and json_diff filters

* fix copyright notices

* fix documentation

* fix docs, add maintainer

* fix review remarks

* add integration test

* fix docs (positional)

* add input validation

* formatting fixes

* more typing tweaks

* documentation fix

* fix review comments

* simplicfy input checking

* accept bytes and bytearray input

* add the fail_test argument

* fix docs format

* fix typing hints

* remove unneeded __future__ imports

(cherry picked from commit f5c1b9c)

Co-authored-by: Stanislav Meduna <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-10 Automatically create a backport for the stable-10 branch integration tests/integration new_contributor Help guide this first time contributor new_plugin New plugin plugins plugin (any type) tests tests unit tests/unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants