Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

[BUG]: Need to remove the dependency on com.github.vmg.protogen:protogen-annotations #2446

Closed
buzz08 opened this issue Sep 3, 2021 · 13 comments
Labels
type: bug bugs/ bug fixes

Comments

@buzz08
Copy link

buzz08 commented Sep 3, 2021

Describe the bug
Need to remove the dependency on com.github.vmg.protogen:protogen-annotations

  • This library is not actively maintained and the Jars are only published to jcenter
  • jcenter is deprecated and this library is no longer accessible
  • It seems like a personal project which is not getting any updates

Details
Conductor version: Latest

To Reproduce
Steps to reproduce the behavior:
Include the client dependencies in a new project and it complains this Jar cannot be accessed when building and fails. Without removing this dependency, it will be hard to any new client to onboard

Expected behavior
Adding conductor client with the following configuration should work without issues

implementation ('com.netflix.conductor:conductor-client:3.0.6')
implementation ('com.netflix.conductor:conductor-common:3.0.6')

Screenshots

Could not find com.github.vmg.protogen:protogen-annotations:1.0.0.
Required by:
    project : > com.netflix.conductor:conductor-common:3.0.6

Additional context
NA

@buzz08 buzz08 added the type: bug bugs/ bug fixes label Sep 3, 2021
@buzz08
Copy link
Author

buzz08 commented Sep 4, 2021

@apanicker-nflx @kishorebanala cc: @cyzhao - Please suggest the recommended fix approach and one of us can try to submit a PR to expedite this. Thanks! cc:

@buzz08
Copy link
Author

buzz08 commented Sep 9, 2021

@kishorebanala - Thoughts on this one? Suggest the approach and we can submit a PR.

marosmars added a commit to FRINXio/conductor that referenced this issue Sep 13, 2021
Quickfix to: Netflix#2446

... just add jcenter repo (it should work for downloads) since netflix
repo no longer holds some of the dependencies of conductor

Signed-off-by: Maros Marsalek <mmarsalek@frinx.io>
@marosmars
Copy link
Contributor

Hey, I temporarily fixed this for myself:

git diff HEAD~1
diff --git a/build.gradle b/build.gradle
index ef4dc41a..1a1ac05c 100644
--- a/build.gradle
+++ b/build.gradle
@@ -75,16 +75,10 @@ allprojects {
          * This repository locates artifacts that don't exist in maven central but we had to backup from jcenter
          * The exclusiveContent
          */
-        exclusiveContent {
-            forRepository {
                 maven {
                     url "https://artifactory-oss.prod.netflix.net/artifactory/required-jcenter-modules-backup"
                 }
-            }
-            filter {
-                includeGroupByRegex "com\\.github\\.vmg.*"
-            }
-        }
+       jcenter()
     }
 
     dependencyManagement {

I just added jcenter repo and removed exlcusivity for netflix/required-jcenter-modules-backup.
I am not sure what is the deal with jcenter vs netflix/required-jcenter-modules-backup and why the dependency suddenly disappeared from netflix/required-jcenter-modules-backup...

Jozefiel pushed a commit to FRINXio/conductor that referenced this issue Sep 13, 2021
Quickfix to: Netflix#2446

... just add jcenter repo (it should work for downloads) since netflix
repo no longer holds some of the dependencies of conductor

Signed-off-by: Maros Marsalek <mmarsalek@frinx.io>
@aravindanr
Copy link
Collaborator

@buzz08 Unfortunately, its not easy to remove that dependency since the grpc module depends on it. Did you try excluding the dependency in your build.gradle? The dependency is used to generate proto files and mappers from Java classes.

@buzz08
Copy link
Author

buzz08 commented Sep 13, 2021

@aravindanr - We can copy the relevant code to the conductor code base and prevent this. It's a bit odd for a new user to have to explicitly exclude dependencies.

If you'd like I can make a patch based on this approach.

Also thanks @marosmars for your suggestion. This should also work for the time being.

@aravindanr
Copy link
Collaborator

aravindanr commented Sep 13, 2021

Adding the code to the Conductor repo was considered but dropped, because that places the maintenance of the copied code on Conductor community.

@vmg Can you publish protogen-annotations to Maven central?

@buzz08
Copy link
Author

buzz08 commented Sep 13, 2021

Hey @aravindanr - What you say makes total sense - One other angle to consider is reducing / removing the dependency on this external library will likely reduce the overall long term maintenance overhead. The library you are referring to can be parked perhaps in the contribs module?

I'll leave this to you to decide, but hopefully you can reduce risks around keeping this library around.

@vmg
Copy link
Contributor

vmg commented Sep 14, 2021

Hey! I'm sorry, I wrote this dependency as part of Conductor's introduction to GitHub and I'm no longer working there. I think the best bet for long-term maintenance would be to park it on contrib. The functionality of the library is quite straightforward (generating ProtoBuf Java objects from Conductor's Java POJOs), but manually doing the same thing would quickly become very cumbersome, since Conductor has a very large API surface in POJOs and it needs to expose it all via GRPC.

The licenses between the projects should be compatible so that won't be an issue. cheers!

@aravindanr
Copy link
Collaborator

aravindanr commented Sep 20, 2021

Parking it in contrib creates a circular dependency between contrib and common.

@buzz08 I wonder if changing the protogen-annotations to compileOnly in conductor-common solves the problem.

@aravindanr
Copy link
Collaborator

@vmg After some thought, we can bring the protogen-annotations as its own module into Conductor, but we won't be able to keep the package names as-is.

com.github.vmg will change to com.netflix.conductor and you will be credited as author. Let us know if you are ok with this.

@vmg
Copy link
Contributor

vmg commented Sep 30, 2021

@aravindanr Absolutely, that's not a problem at all.

@boney9
Copy link
Contributor

boney9 commented Nov 13, 2021

@aravindanr Did we get a chance to make this change?

@boney9
Copy link
Contributor

boney9 commented Nov 15, 2021

@aravindanr - I took a stab at moving this code with this PR - it works locally and it was an easy change - but the PR build seems to fail to recognize the new modules - not sure what is happening - will you be able to take a look?

The CI/CD is working on the PR - have a look and let me know if we can merge this.

#2575

cc @v1r3n

marosmars added a commit to FRINXio/conductor that referenced this issue Feb 22, 2022
Quickfix to: Netflix#2446

... just add jcenter repo (it should work for downloads) since netflix
repo no longer holds some of the dependencies of conductor

Signed-off-by: Maros Marsalek <mmarsalek@frinx.io>
Jozefiel pushed a commit to FRINXio/conductor that referenced this issue Oct 11, 2022
Quickfix to: Netflix#2446

... just add jcenter repo (it should work for downloads) since netflix
repo no longer holds some of the dependencies of conductor

Signed-off-by: Maros Marsalek <mmarsalek@frinx.io>
marosmars added a commit to FRINXio/conductor that referenced this issue Dec 5, 2022
Quickfix to: Netflix#2446

... just add jcenter repo (it should work for downloads) since netflix
repo no longer holds some of the dependencies of conductor

Signed-off-by: Maros Marsalek <mmarsalek@frinx.io>
Jozefiel pushed a commit to FRINXio/conductor that referenced this issue Feb 2, 2023
Quickfix to: Netflix#2446

... just add jcenter repo (it should work for downloads) since netflix
repo no longer holds some of the dependencies of conductor

Signed-off-by: Maros Marsalek <mmarsalek@frinx.io>
JumasJM pushed a commit to FRINXio/conductor that referenced this issue Sep 27, 2023
Quickfix to: Netflix#2446

... just add jcenter repo (it should work for downloads) since netflix
repo no longer holds some of the dependencies of conductor

Signed-off-by: Maros Marsalek <mmarsalek@frinx.io>
JumasJM pushed a commit to FRINXio/conductor that referenced this issue Oct 18, 2023
Quickfix to: Netflix#2446

... just add jcenter repo (it should work for downloads) since netflix
repo no longer holds some of the dependencies of conductor

Signed-off-by: Maros Marsalek <mmarsalek@frinx.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug bugs/ bug fixes
Projects
None yet
Development

No branches or pull requests

5 participants