#449 Re-add OSGi headers to all modules. by raulk · Pull Request #450 · OpenFeign/feign

raulk · 2016-08-26T15:49:33Z

Via the maven-bundle-plugin.

spencergibb · 2016-08-26T16:12:56Z

@raulk how do I verify they were added?

spencergibb · 2016-08-26T16:21:52Z

I see the following in META-INF/MANIFEST.MF:

Manifest-Version: 1.0
Bundle-Description: Feign Core
Bundle-License: http://www.apache.org/licenses/LICENSE-2.0.txt
Bundle-SymbolicName: io.github.openfeign.feign-core
Archiver-Version: Plexus Archiver
Built-By: sgibb
Bnd-LastModified: 1472228033106
Bundle-ManifestVersion: 2
Bundle-DocURL: https://github.com/openfeign
Bundle-Vendor: OpenFeign
Import-Package: feign,feign.codec,javax.net.ssl
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.6))"
Tool: Bnd-3.2.0.201605172007
Export-Package: feign.auth;uses:=feign;version="9.3.1",feign;uses:="fe
 ign.codec,javax.net.ssl";version="9.3.1",feign.codec;uses:=feign;vers
 ion="9.3.1"
Bundle-Name: Feign Core
Bundle-Version: 9.3.1.SNAPSHOT
Created-By: Apache Maven Bundle Plugin
Build-Jdk: 1.8.0_91

From 8.17.0 I see

Manifest-Version: 1.0
Build-Date: 2016-06-05_00:09:03
Bundle-SymbolicName: com.netflix.feign.core
Build-Number: LOCAL
Built-By: travis
Bnd-LastModified: 1465085374000
Import-Package: javax.net.ssl
Branch: 1c1d2fa91ed0788cad7a5a9901feb22cbf3de8c3
Gradle-Version: 2.2.1
Export-Package: feign;version="8.17.0";uses:="feign.codec,javax.net.ss
 l",feign.auth;version="8.17.0";uses:=feign,feign.codec;version="8.17.
 0";uses:=feign
Built-OS: Linux
Bundle-Name: feign-core
Build-Host: testing-worker-linux-docker-f919d37f-3389-linux-2
Module-Email: talent@netflix.com
Implementation-Title: com.netflix.feign#feign-core;8.17.0
Module-Origin: https://github.com/Netflix/feign.git
Build-Id: LOCAL
Implementation-Version: 8.17.0
Module-Owner: talent@netflix.com
Bundle-ManifestVersion: 2
Change: 1c1d2fa
Module-Source: /core
Tool: Bnd-2.1.0.20130426-122213
Built-Status: integration
Build-Job: LOCAL
Bundle-Version: 8.17.0
X-Compile-Target-JDK: 1.6
X-Compile-Source-JDK: 1.6
Created-By: 1.8.0_31-b13 (Oracle Corporation)
Build-Java-Version: 1.8.0_31

I'd like a plus one from someone not me and not the author that the above is sufficient.

dsyer · 2016-08-26T17:37:04Z

IIRC the only thing that really matters is the Export/Import-Package and that looks sane. If the author says it works I think that's good enough for me. But how are we generating this data (the versions must change for every release)?

spencergibb · 2016-08-26T17:43:31Z

@dsyer org.apache.felix:maven-bundle-plugin maven plugin. I just ran a normal build against this PR and got the first MANIFEST.MF.

codefromthecrypt · 2016-08-27T01:14:07Z

I don't understand the nuance of import export, but is it normal for feign
to export javax.net.ssl?

On Sat, Aug 27, 2016 at 12:21 AM, Spencer Gibb notifications@github.com
wrote:

I see the following in META-INF/MANIFEST.MF:

Manifest-Version: 1.0
Bundle-Description: Feign Core
Bundle-License: http://www.apache.org/licenses/LICENSE-2.0.txt
Bundle-SymbolicName: io.github.openfeign.feign-core
Archiver-Version: Plexus Archiver
Built-By: sgibb
Bnd-LastModified: 1472228033106
Bundle-ManifestVersion: 2
Bundle-DocURL: https://github.com/openfeign
Bundle-Vendor https://github.com/openfeignBundle-Vendor: OpenFeign
Import-Package: feign,feign.codec,javax.net.ssl
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.6))"
Tool: Bnd-3.2.0.201605172007
Export-Package: feign.auth;uses:=feign;version="9.3.1",feign;uses:="fe
ign.codec,javax.net.ssl";version="9.3.1",feign.codec;uses:=feign;vers
ion="9.3.1"
Bundle-Name: Feign Core
Bundle-Version: 9.3.1.SNAPSHOT
Created-By: Apache Maven Bundle Plugin
Build-Jdk: 1.8.0_91

From 8.17.0 I see

Manifest-Version: 1.0
Build-Date: 2016-06-05_00:09:03
Bundle-SymbolicName: com.netflix.feign.core
Build-Number: LOCAL
Built-By: travis
Bnd-LastModified: 1465085374000
Import-Package: javax.net.ssl
Branch: 1c1d2fa
Gradle-Version: 2.2.1
Export-Package: feign;version="8.17.0";uses:="feign.codec,javax.net.ss
l",feign.auth;version="8.17.0";uses:=feign,feign.codec;version="8.17.
0";uses:=feign
Built-OS: Linux
Bundle-Name: feign-core
Build-Host: testing-worker-linux-docker-f919d37f-3389-linux-2
Module-Email: talent@netflix.com
Implementation-Title: com.netflix.feign#feign-core;8.17.0
Module-Origin: https://github.com/Netflix/feign.git
Build-Id https://github.com/Netflix/feign.gitBuild-Id: LOCAL
Implementation-Version: 8.17.0
Module-Owner: talent@netflix.com
Bundle-ManifestVersion: 2
Change: 1c1d2fa
Module-Source: /core
Tool: Bnd-2.1.0.20130426-122213
Built-Status: integration
Build-Job: LOCAL
Bundle-Version: 8.17.0
X-Compile-Target-JDK: 1.6
X-Compile-Source-JDK: 1.6
Created-By: 1.8.0_31-b13 (Oracle Corporation)
Build-Java-Version: 1.8.0_31

I'd like a plus one from someone not me and not the author that the above
is sufficient.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#450 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD61y_gYZv3JUEqz9k-OQ8_0L5x18QOks5qjxKhgaJpZM4JuNjg
.

raulk · 2016-08-28T23:11:48Z

OSGi has a select set of headers, out of which the important ones are
Bundle-SymbolicName, Import-Package, Export-Package and a few others.

Many of the manifest headers in 8.18.0 were not OSGi related. I suppose
they were added by the Nebula plugins as metadata.

We do not export javax.net.ssl. If you look closely, that package is part
of a "uses" clause of an export, which is merely used by OSGi to calculate
a consistent class space.

Version ranges are updated automatically by the maven-bundle-plugin as the
POM version evolves. There's nothing additional to take care of.

On 27 Aug 2016 02:14, "Adrian Cole" notifications@github.com wrote:

I don't understand the nuance of import export, but is it normal for feign
to export javax.net.ssl?

On Sat, Aug 27, 2016 at 12:21 AM, Spencer Gibb notifications@github.com
wrote:

I see the following in META-INF/MANIFEST.MF:

Manifest-Version: 1.0
Bundle-Description: Feign Core
Bundle-License: http://www.apache.org/licenses/LICENSE-2.0.txt
Bundle-SymbolicName: io.github.openfeign.feign-core
Archiver-Version: Plexus Archiver
Built-By: sgibb
Bnd-LastModified: 1472228033106
Bundle-ManifestVersion: 2
Bundle-DocURL: https://github.com/openfeign
Bundle-Vendor https://github.com/openfeignBundle-Vendor: OpenFeign
Import-Package: feign,feign.codec,javax.net.ssl
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.6))"
Tool: Bnd-3.2.0.201605172007
Export-Package: feign.auth;uses:=feign;version="9.3.1",feign;uses:="fe
ign.codec,javax.net.ssl";version="9.3.1",feign.codec;uses:=feign;vers
ion="9.3.1"
Bundle-Name: Feign Core
Bundle-Version: 9.3.1.SNAPSHOT
Created-By: Apache Maven Bundle Plugin
Build-Jdk: 1.8.0_91

From 8.17.0 I see

Manifest-Version: 1.0
Build-Date: 2016-06-05_00:09:03
Bundle-SymbolicName: com.netflix.feign.core
Build-Number: LOCAL
Built-By: travis
Bnd-LastModified: 1465085374000
Import-Package: javax.net.ssl
Branch: 1c1d2fa
Gradle-Version: 2.2.1
Export-Package: feign;version="8.17.0";uses:="feign.codec,javax.net.ss
l",feign.auth;version="8.17.0";uses:=feign,feign.codec;version="8.17.
0";uses:=feign
Built-OS: Linux
Bundle-Name: feign-core
Build-Host: testing-worker-linux-docker-f919d37f-3389-linux-2
Module-Email: talent@netflix.com
Implementation-Title: com.netflix.feign#feign-core;8.17.0
Module-Origin: https://github.com/Netflix/feign.git
Build-Id https://github.com/Netflix/feign.gitBuild-Id: LOCAL
Implementation-Version: 8.17.0
Module-Owner: talent@netflix.com
Bundle-ManifestVersion: 2
Change: 1c1d2fa
Module-Source: /core
Tool: Bnd-2.1.0.20130426-122213
Built-Status: integration
Build-Job: LOCAL
Bundle-Version: 8.17.0
X-Compile-Target-JDK: 1.6
X-Compile-Source-JDK: 1.6
Created-By: 1.8.0_31-b13 (Oracle Corporation)
Build-Java-Version: 1.8.0_31

I'd like a plus one from someone not me and not the author that the above
is sufficient.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#450 (comment),
or mute
the thread
<https://github.com/notifications/unsubscribe-
auth/AAD61y_gYZv3JUEqz9k-OQ8_0L5x18QOks5qjxKhgaJpZM4JuNjg>
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#450 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABDNutasQzUmOiNdDZslnZJ4g1dVYj6_ks5qj49pgaJpZM4JuNjg
.

codefromthecrypt · 2016-08-28T23:55:10Z

Ahh ok.

Any way to test this so that we dont break it again?

Ex. an invoker test in src/main/it or something?

On 29 Aug 2016 07:11, "Raúl Kripalani" notifications@github.com wrote:

OSGi has a select set of headers, out of which the important ones are
Bundle-SymbolicName, Import-Package, Export-Package and a few others.

Many of the manifest headers in 8.18.0 were not OSGi related. I suppose
they were added by the Nebula plugins as metadata.

We do not export javax.net.ssl. If you look closely, that package is part
of a "uses" clause of an export, which is merely used by OSGi to calculate
a consistent class space.

Version ranges are updated automatically by the maven-bundle-plugin as the
POM version evolves. There's nothing additional to take care of.

On 27 Aug 2016 02:14, "Adrian Cole" notifications@github.com wrote:

I don't understand the nuance of import export, but is it normal for
feign
to export javax.net.ssl?

On Sat, Aug 27, 2016 at 12:21 AM, Spencer Gibb <notifications@github.com

wrote:

I see the following in META-INF/MANIFEST.MF:

Manifest-Version: 1.0
Bundle-Description: Feign Core
Bundle-License: http://www.apache.org/licenses/LICENSE-2.0.txt
Bundle-SymbolicName: io.github.openfeign.feign-core
Archiver-Version: Plexus Archiver
Built-By: sgibb
Bnd-LastModified: 1472228033106
Bundle-ManifestVersion: 2
Bundle-DocURL: https://github.com/openfeign
Bundle-Vendor https://github.com/openfeignBundle-Vendor: OpenFeign
Import-Package: feign,feign.codec,javax.net.ssl
Require-Capability: osgi.ee;filter:="(&(osgi.ee=JavaSE)(version=1.6))"
Tool: Bnd-3.2.0.201605172007
Export-Package: feign.auth;uses:=feign;version="9.3.1",feign;uses:="fe
ign.codec,javax.net.ssl";version="9.3.1",feign.codec;uses:=feign;vers
ion="9.3.1"
Bundle-Name: Feign Core
Bundle-Version: 9.3.1.SNAPSHOT
Created-By: Apache Maven Bundle Plugin
Build-Jdk: 1.8.0_91

From 8.17.0 I see

Manifest-Version: 1.0
Build-Date: 2016-06-05_00:09:03
Bundle-SymbolicName: com.netflix.feign.core
Build-Number: LOCAL
Built-By: travis
Bnd-LastModified: 1465085374000
Import-Package: javax.net.ssl
Branch: 1c1d2fa
Gradle-Version: 2.2.1
Export-Package: feign;version="8.17.0";uses:="feign.codec,javax.net.ss
l",feign.auth;version="8.17.0";uses:=feign,feign.codec;version="8.17.
0";uses:=feign
Built-OS: Linux
Bundle-Name: feign-core
Build-Host: testing-worker-linux-docker-f919d37f-3389-linux-2
Module-Email: talent@netflix.com
Implementation-Title: com.netflix.feign#feign-core;8.17.0
Module-Origin: https://github.com/Netflix/feign.git
Build-Id https://github.com/Netflix/feign.gitBuild-Id: LOCAL
Implementation-Version: 8.17.0
Module-Owner: talent@netflix.com
Bundle-ManifestVersion: 2
Change: 1c1d2fa
Module-Source: /core
Tool: Bnd-2.1.0.20130426-122213
Built-Status: integration
Build-Job: LOCAL
Bundle-Version: 8.17.0
X-Compile-Target-JDK: 1.6
X-Compile-Source-JDK: 1.6
Created-By: 1.8.0_31-b13 (Oracle Corporation)
Build-Java-Version: 1.8.0_31

I'd like a plus one from someone not me and not the author that the
above
is sufficient.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#450 (comment),
or mute
the thread
<https://github.com/notifications/unsubscribe-
auth/AAD61y_gYZv3JUEqz9k-OQ8_0L5x18QOks5qjxKhgaJpZM4JuNjg>
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#450 (comment),
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/
ABDNutasQzUmOiNdDZslnZJ4g1dVYj6_ks5qj49pgaJpZM4JuNjg>
.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#450 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD61--zv2nc9XQE70DiDAl7agHK1scYks5qkhW1gaJpZM4JuNjg
.

raulk · 2016-08-28T23:57:10Z

Hmmm... We could consider a Pax Exam test. I'll look into that separately.
Would appreciate if you can merge the PR for now without a regression test.

On 29 Aug 2016 00:55, "Adrian Cole" notifications@github.com wrote:

Ahh ok.

Any way to test this so that we dont break it again?

Ex. an invoker test in src/main/it or something?

On 29 Aug 2016 07:11, "Raúl Kripalani" notifications@github.com wrote:

OSGi has a select set of headers, out of which the important ones are
Bundle-SymbolicName, Import-Package, Export-Package and a few others.

Many of the manifest headers in 8.18.0 were not OSGi related. I suppose
they were added by the Nebula plugins as metadata.

We do not export javax.net.ssl. If you look closely, that package is part
of a "uses" clause of an export, which is merely used by OSGi to
calculate
a consistent class space.

Version ranges are updated automatically by the maven-bundle-plugin as
the
POM version evolves. There's nothing additional to take care of.

On 27 Aug 2016 02:14, "Adrian Cole" notifications@github.com wrote:

I don't understand the nuance of import export, but is it normal for
feign
to export javax.net.ssl?

On Sat, Aug 27, 2016 at 12:21 AM, Spencer Gibb <
notifications@github.com

wrote:

I see the following in META-INF/MANIFEST.MF:

Manifest-Version: 1.0
Bundle-Description: Feign Core
Bundle-License: http://www.apache.org/licenses/LICENSE-2.0.txt
Bundle-SymbolicName: io.github.openfeign.feign-core
Archiver-Version: Plexus Archiver
Built-By: sgibb
Bnd-LastModified: 1472228033106
Bundle-ManifestVersion: 2
Bundle-DocURL: https://github.com/openfeign
Bundle-Vendor https://github.com/openfeignBundle-Vendor: OpenFeign
Import-Package: feign,feign.codec,javax.net.ssl
Require-Capability: osgi.ee;filter:="(&(osgi.ee=
JavaSE)(version=1.6))"
Tool: Bnd-3.2.0.201605172007
Export-Package: feign.auth;uses:=feign;version="9.3.1",feign;uses:="
fe
ign.codec,javax.net.ssl";version="9.3.1",feign.codec;
uses:=feign;vers
ion="9.3.1"
Bundle-Name: Feign Core
Bundle-Version: 9.3.1.SNAPSHOT
Created-By: Apache Maven Bundle Plugin
Build-Jdk: 1.8.0_91

From 8.17.0 I see

Manifest-Version: 1.0
Build-Date: 2016-06-05_00:09:03
Bundle-SymbolicName: com.netflix.feign.core
Build-Number: LOCAL
Built-By: travis
Bnd-LastModified: 1465085374000
Import-Package: javax.net.ssl
Branch: 1c1d2fa
Gradle-Version: 2.2.1
Export-Package: feign;version="8.17.0";uses:="
feign.codec,javax.net.ss
l",feign.auth;version="8.17.0";uses:=feign,feign.codec;
version="8.17.
0";uses:=feign
Built-OS: Linux
Bundle-Name: feign-core
Build-Host: testing-worker-linux-docker-f919d37f-3389-linux-2
Module-Email: talent@netflix.com
Implementation-Title: com.netflix.feign#feign-core;8.17.0
Module-Origin: https://github.com/Netflix/feign.git
Build-Id https://github.com/Netflix/feign.gitBuild-Id: LOCAL
Implementation-Version: 8.17.0
Module-Owner: talent@netflix.com
Bundle-ManifestVersion: 2
Change: 1c1d2fa
Module-Source: /core
Tool: Bnd-2.1.0.20130426-122213
Built-Status: integration
Build-Job: LOCAL
Bundle-Version: 8.17.0
X-Compile-Target-JDK: 1.6
X-Compile-Source-JDK: 1.6
Created-By: 1.8.0_31-b13 (Oracle Corporation)
Build-Java-Version: 1.8.0_31

I'd like a plus one from someone not me and not the author that the
above
is sufficient.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#450 (comment)
,
or mute
the thread
<https://github.com/notifications/unsubscribe-
auth/AAD61y_gYZv3JUEqz9k-OQ8_0L5x18QOks5qjxKhgaJpZM4JuNjg>
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#450 (comment),
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/
ABDNutasQzUmOiNdDZslnZJ4g1dVYj6_ks5qj49pgaJpZM4JuNjg>
.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#450 (comment),
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAD61--
zv2nc9XQE70DiDAl7agHK1scYks5qkhW1gaJpZM4JuNjg>
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#450 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABDNuqpS0BgTex0VOa8P7V08yGdXfGxmks5qkh_pgaJpZM4JuNjg
.

codefromthecrypt · 2016-08-29T00:06:26Z

Yeah sure. I think i remember PAM used in jclouds and it did catch when
people like me screwed up things. Either that or you just let us know next
time someone messes it up!

On 29 Aug 2016 07:57, "Raúl Kripalani" notifications@github.com wrote:

Hmmm... We could consider a Pax Exam test. I'll look into that separately.
Would appreciate if you can merge the PR for now without a regression test.

On 29 Aug 2016 00:55, "Adrian Cole" notifications@github.com wrote:

Ahh ok.

Any way to test this so that we dont break it again?

Ex. an invoker test in src/main/it or something?

On 29 Aug 2016 07:11, "Raúl Kripalani" notifications@github.com wrote:

OSGi has a select set of headers, out of which the important ones are
Bundle-SymbolicName, Import-Package, Export-Package and a few others.

Many of the manifest headers in 8.18.0 were not OSGi related. I suppose
they were added by the Nebula plugins as metadata.

We do not export javax.net.ssl. If you look closely, that package is
part
of a "uses" clause of an export, which is merely used by OSGi to
calculate
a consistent class space.

Version ranges are updated automatically by the maven-bundle-plugin as
the
POM version evolves. There's nothing additional to take care of.

On 27 Aug 2016 02:14, "Adrian Cole" notifications@github.com wrote:

I don't understand the nuance of import export, but is it normal for
feign
to export javax.net.ssl?

On Sat, Aug 27, 2016 at 12:21 AM, Spencer Gibb <
notifications@github.com

wrote:

I see the following in META-INF/MANIFEST.MF:

Manifest-Version: 1.0
Bundle-Description: Feign Core
Bundle-License: http://www.apache.org/licenses/LICENSE-2.0.txt
Bundle-SymbolicName: io.github.openfeign.feign-core
Archiver-Version: Plexus Archiver
Built-By: sgibb
Bnd-LastModified: 1472228033106
Bundle-ManifestVersion: 2
Bundle-DocURL: https://github.com/openfeign
Bundle-Vendor https://github.com/openfeignBundle-Vendor:
OpenFeign
Import-Package: feign,feign.codec,javax.net.ssl
Require-Capability: osgi.ee;filter:="(&(osgi.ee=
JavaSE)(version=1.6))"
Tool: Bnd-3.2.0.201605172007
Export-Package: feign.auth;uses:=feign;
version="9.3.1",feign;uses:="
fe
ign.codec,javax.net.ssl";version="9.3.1",feign.codec;
uses:=feign;vers
ion="9.3.1"
Bundle-Name: Feign Core
Bundle-Version: 9.3.1.SNAPSHOT
Created-By: Apache Maven Bundle Plugin
Build-Jdk: 1.8.0_91

From 8.17.0 I see

Manifest-Version: 1.0
Build-Date: 2016-06-05_00:09:03
Bundle-SymbolicName: com.netflix.feign.core
Build-Number: LOCAL
Built-By: travis
Bnd-LastModified: 1465085374000
Import-Package: javax.net.ssl
Branch: 1c1d2fa
Gradle-Version: 2.2.1
Export-Package: feign;version="8.17.0";uses:="
feign.codec,javax.net.ss
l",feign.auth;version="8.17.0";uses:=feign,feign.codec;
version="8.17.
0";uses:=feign
Built-OS: Linux
Bundle-Name: feign-core
Build-Host: testing-worker-linux-docker-f919d37f-3389-linux-2
Module-Email: talent@netflix.com
Implementation-Title: com.netflix.feign#feign-core;8.17.0
Module-Origin: https://github.com/Netflix/feign.git
Build-Id https://github.com/Netflix/feign.gitBuild-Id: LOCAL
Implementation-Version: 8.17.0
Module-Owner: talent@netflix.com
Bundle-ManifestVersion: 2
Change: 1c1d2fa
Module-Source: /core
Tool: Bnd-2.1.0.20130426-122213
Built-Status: integration
Build-Job: LOCAL
Bundle-Version: 8.17.0
X-Compile-Target-JDK: 1.6
X-Compile-Source-JDK: 1.6
Created-By: 1.8.0_31-b13 (Oracle Corporation)
Build-Java-Version: 1.8.0_31

I'd like a plus one from someone not me and not the author that the
above
is sufficient.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#450 (comment)
242781789
,
or mute
the thread
<https://github.com/notifications/unsubscribe-
auth/AAD61y_gYZv3JUEqz9k-OQ8_0L5x18QOks5qjxKhgaJpZM4JuNjg>
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#450 (comment)
,
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/
ABDNutasQzUmOiNdDZslnZJ4g1dVYj6_ks5qj49pgaJpZM4JuNjg>
.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#450 (comment),
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAD61--
zv2nc9XQE70DiDAl7agHK1scYks5qkhW1gaJpZM4JuNjg>
.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#450 (comment),
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/
ABDNuqpS0BgTex0VOa8P7V08yGdXfGxmks5qkh_pgaJpZM4JuNjg>
.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#450 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD61_hjR-A30HQAKhNBzDseUXGAzkrjks5qkiBXgaJpZM4JuNjg
.

codefromthecrypt · 2016-08-29T00:07:01Z

LGTM

ravihansa3000 · 2016-08-29T00:43:43Z

This doesn't work with google-gson 2.7 in an actual OSGi environment, Eclipse P2 OSGi container to be precise. Reason being feign-gson module has imports to google-gson internal packages [1] which are not exported from google-gson. google-gson has been explicitly instructed not to export internal packages [2].

Problem is not with this PR, but we need to fix DoubleToIntMapTypeAdapter before anyone could make use of this.

[1] https://github.com/OpenFeign/feign/blob/master/gson/src/main/java/feign/gson/DoubleToIntMapTypeAdapter.java#L21-L22

[2] https://github.com/google/gson/blob/master/gson/pom.xml#L28

codefromthecrypt · 2016-08-29T01:00:25Z

we are not required to make all things work in OSGi, particularly as it is
a minority of the ecosystem that care about it. We can just add a note to
the gson README saying that if you care about OSGi, then don't use the
default constructor for the gson adapters, as they use an internal class

GsonDecoder(Gson gson)

codefromthecrypt · 2016-08-29T01:16:41Z

@raulk ping back if you need to make an adjustment in maven config to exclude the gson internal class on that module. I raised #451 to document this, as it has existed in this way since feign was created

ravihansa3000 · 2016-08-29T05:18:34Z

IMHO if we accept to make OpenFeign OSGi ready then it should be required to work in OSGi environment. Or else it would be better not to have it at all.
Do you think it's a good idea to have feign-gson as a fragment bundle of google-gson host bundle?

codefromthecrypt · 2016-08-29T05:42:24Z

Opinion noted, and if strict OSGi compliance for all aspects of all code
becomes a dominant opinion we can revisit.

For now, you can either choose to use the workaround, choose to put in
effort to make the internal classes internal, or choose not to use the gson
module.

On 29 Aug 2016 13:18, "Akila Ravihansa Perera" notifications@github.com
wrote:

IMHO if we accept to make OpenFeign OSGi ready then it should be required
to work in OSGi environment. Or else it would be better not to have it at
all.
Do you think it's a good idea to have feign-gson as a fragment bundle of
google-gson host bundle?

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#450 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAD618vxkTh5KTtxEVC334veYzxRZqY7ks5qkmurgaJpZM4JuNjg
.

raulk · 2016-08-29T11:51:44Z

My goal with this ticket was to fix a regression whereby newest versions of Feign stopped generating OSGi headers altogether.

We could discuss about OSGi in a broader context – and how to make things nicer for Gson, but I don't think it should prevent this PR from being merged.

raulk · 2016-08-29T15:03:24Z

@ravihansa3000 – I wouldn't deploy feign-gson as an OSGi fragment of Gson, that makes little sense technically speaking, because your fragment would have to declare the feign.gson export which would result in the Gson bundle exporting feign.gson.

If anything, I would consider creating a little helper OSGi fragment that does nothing but attach to Gson and export the internal package you need, e.g. feign-gson-osgi-helper.

That or change feign-gson so that it doesn't rely on internal classes of Gson, if at all possible.

raulk · 2016-08-29T15:03:43Z

@spencergibb – thanks for the merge 👍

ravihansa3000 · 2016-08-30T00:43:34Z

@adriancole thank you for suggesting a workaround.

@raulk I agree. I'll try to initiate a discussion on this in Gitter.

OpenFeign#449 Re-add OSGi headers to all modules.

a95bc6f

codefromthecrypt mentioned this pull request Aug 29, 2016

Adds note about gson map decoding to README #451

Merged

spencergibb merged commit ced133e into OpenFeign:master Aug 29, 2016

raulk deleted the feature/osgi-headers branch August 29, 2016 15:03

velo pushed a commit that referenced this pull request Oct 7, 2024

#449 Re-add OSGi headers to all modules. (#450)

a24e631

velo pushed a commit that referenced this pull request Oct 8, 2024

#449 Re-add OSGi headers to all modules. (#450)

3e7231d

Conversation

raulk commented Aug 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spencergibb commented Aug 26, 2016

Uh oh!

spencergibb commented Aug 26, 2016

Uh oh!

dsyer commented Aug 26, 2016

Uh oh!

spencergibb commented Aug 26, 2016

Uh oh!

codefromthecrypt commented Aug 27, 2016

Uh oh!

raulk commented Aug 28, 2016

Uh oh!

codefromthecrypt commented Aug 28, 2016

Uh oh!

raulk commented Aug 28, 2016

Uh oh!

codefromthecrypt commented Aug 29, 2016

Uh oh!

codefromthecrypt commented Aug 29, 2016 via email

Uh oh!

ravihansa3000 commented Aug 29, 2016

Uh oh!

codefromthecrypt commented Aug 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codefromthecrypt commented Aug 29, 2016

Uh oh!

ravihansa3000 commented Aug 29, 2016

Uh oh!

codefromthecrypt commented Aug 29, 2016

Uh oh!

raulk commented Aug 29, 2016

Uh oh!

raulk commented Aug 29, 2016

Uh oh!

raulk commented Aug 29, 2016

Uh oh!

ravihansa3000 commented Aug 30, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

raulk commented Aug 26, 2016 •

edited

Loading

codefromthecrypt commented Aug 29, 2016 •

edited

Loading