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

feat(zjsonpatch): move io.fabric8:zjsonpatch as a module in Fabric8 kubernetes Client #6276

Merged
merged 2 commits into from
Sep 3, 2024

Conversation

rohanKanojia
Copy link
Member

@rohanKanojia rohanKanojia commented Aug 14, 2024

Description

Fix #5480
Fix #3489

Supersedes closes #4700

Currently, we rely on outdated io.fabric8:zjsonpatch dependency instead of the upstream com.flipkart.zjsonpatch:zjsonpatch dependency. The main issue is that the upstream repository depends on org.apache.commons:commons-collections4 .

  • Add a new module zjsonpatch that would directly include com.flipkart.zjsonpatch:zjsonpatch dependency excluding org.apache.commons:commons-collections4.
  • Port required Apache Commons Collections4 method ListUtils.longestCommonSubsequence and related
    classes inside zjsonpatch module

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change
  • Chore (non-breaking change which doesn't affect codebase;
    test, version modification, documentation, etc.)

Checklist

  • Code contributed by me aligns with current project license: Apache 2.0
  • I Added CHANGELOG entry regarding this change
  • I have implemented unit tests to cover my changes
  • I have added/updated the javadocs and other documentation accordingly
  • No new bugs, code smells, etc. in SonarCloud report
  • I tested my code in Kubernetes
  • I tested my code in OpenShift

@rohanKanojia rohanKanojia force-pushed the pr/issue5480 branch 5 times, most recently from 9272d1d to 23f3ea0 Compare August 16, 2024 04:35
@rohanKanojia
Copy link
Member Author

After migrating to the upstream com.flipkart.zjsonpatch dependency, KubernetesMutationHookHandlingTest started failing. Upon checking the failures I found that this module depends on Java Operator SDK that uses io.fabric8:zjsonpatch library to calculate JSON diff in GenericKubernetesResourceMatcher.

Adding back the io.fabric8.zjsonpatch.JsonDiff fixes the failing test.

@rohanKanojia rohanKanojia force-pushed the pr/issue5480 branch 4 times, most recently from 3e03b82 to ef75950 Compare August 16, 2024 15:21
}

public static JsonNode asJson(final JsonNode source, final JsonNode target) {
return com.flipkart.zjsonpatch.JsonDiff.asJson(source, target);
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to reintroduce this class due to #6276 (comment)

@rohanKanojia rohanKanojia marked this pull request as ready for review August 26, 2024 10:20
Copy link
Contributor

@shawkins shawkins 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 @rohanKanojia

@manusa
Copy link
Member

manusa commented Aug 27, 2024

Let's discuss this tomorrow. I'm not sure about the current approach and how does this might converge with users relying on commons-collections on their projects.

@fabric8io fabric8io deleted a comment from rohanKanojia Aug 30, 2024
@rohanKanojia rohanKanojia force-pushed the pr/issue5480 branch 2 times, most recently from 848e90f to a3ac63b Compare August 30, 2024 13:57
@rohanKanojia rohanKanojia changed the title chore (deps) : Directly rely on upstream zjsonpatch repository instead of fabric8io fork chore (deps) : Move io.fabric8:zjsonpatch as a module in Fabric8 kubernetes Client Aug 30, 2024
@rohanKanojia rohanKanojia force-pushed the pr/issue5480 branch 2 times, most recently from 614e77b to c86a4bf Compare September 2, 2024 05:02
@manusa manusa self-assigned this Sep 3, 2024
…d of fabric8io fork

+ Add a new module zjsonpatch that would directly include
  `com.flipkart.zjsonpatch:zjsonpatch` dependency excluding
  `org.apache.commons:commons-collections4`.
+ Port required method `ListUtils.longestCommonSubsequence` and related
  classes inside zjsonpatch module

Signed-off-by: Rohan Kumar <[email protected]>
@manusa manusa changed the title chore (deps) : Move io.fabric8:zjsonpatch as a module in Fabric8 kubernetes Client feat(zjsonpatch): move io.fabric8:zjsonpatch as a module in Fabric8 kubernetes Client Sep 3, 2024
Since we're already copying most of the codebase, let's just remove the dependency.

Signed-off-by: Marc Nuri <[email protected]>
@manusa manusa added this to the 7.0.0 milestone Sep 3, 2024 — with automated-tasks
Copy link

sonarqubecloud bot commented Sep 3, 2024

Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

@manusa manusa merged commit 079a7b8 into fabric8io:main Sep 3, 2024
21 checks passed
@rohanKanojia rohanKanojia deleted the pr/issue5480 branch September 3, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSONPatch functionality Critical CVE CVE-2017-7525 in zjsonpatch-0.3.0.jar
4 participants