refactor!: Re-packaging #143
Conversation
fabriziodemaria
force-pushed
the
re-packaging
branch
3 times, most recently
from
06ff942
to
635f80c
Compare
| import com.google.protobuf.Struct; | ||
| import com.google.protobuf.util.Values; |
There was a problem hiding this comment.
This import is meant for test class only, makes maven dependency analyzer unhappy.
I just changed to using another proto API that doesn't upset the analyzer
| import dev.openfeature.sdk.EvaluationContext; | ||
| import dev.openfeature.sdk.Structure; | ||
| import dev.openfeature.sdk.Value; | ||
| import dev.openfeature.sdk.exceptions.TypeMismatchError; | ||
| import io.grpc.netty.shaded.io.netty.util.internal.StringUtil; |
There was a problem hiding this comment.
Nice to remove this dependency for String checks
| @@ -0,0 +1,48 @@ | |||
| package com.spotify.confidence; | |||
There was a problem hiding this comment.
Just copied as is
| import java.util.Map; | ||
| import java.util.concurrent.CompletableFuture; | ||
|
|
||
| public class ResolverClientTestUtils { |
|
|
||
| import java.time.Instant; | ||
|
|
||
| public class FakeClock implements Clock { |
| import java.util.List; | ||
| import java.util.Optional; | ||
|
|
||
| public class FakeEventSenderEngine implements EventSenderEngine { |
fabriziodemaria
force-pushed
the
re-packaging
branch
3 times, most recently
from
a755de6
to
cd479c9
Compare
| <exclusions> | ||
| <exclusion> <!-- declare the exclusion here --> | ||
| <groupId>com.google.guava</groupId> | ||
| <artifactId>guava</artifactId> | ||
| </exclusion> | ||
| </exclusions> |
There was a problem hiding this comment.
Not sure if this transitional dependency is even used by us, but it's marked as vulnerable when a user imports our library. This change removes that warning for our user
|
Now tested that both the JARs (OF and native) work on a sample Java executable. |
| <executions> | ||
| <execution> | ||
| <id>analyze</id> | ||
| <goals> | ||
| <goal>analyze-only</goal> | ||
| </goals> | ||
| <configuration> | ||
| <failOnWarning>true</failOnWarning> | ||
| <failOnWarning>false</failOnWarning> |
There was a problem hiding this comment.
Unfortunate, but the more specific <ignoredNonTestScopedDependencies> added below didn't work for me to make Maven Analyzer succeed.
The root problem seems to be that io.grpc:grpc-stub is used in compile source code, but it's "meant to be used in tests".
|
tested the modules with local jar installed with |
| @@ -2,7 +2,7 @@ | |||
| "bootstrap-sha": "188faf7e47b03bde3bf02c3521771301c77580d6", | |||
| "packages": { | |||
| ".": { | |||
There was a problem hiding this comment.
This . signals that we release (with release please) the whole repo as one release (which means the SDK will bump its number even if we only have changes in the open feature provider).
I guess it would be the easiest approach and maybe we could improve on it later.
fabriziodemaria
force-pushed
the
re-packaging
branch
from
d03ad97
to
a1dd2ed
Compare
fabriziodemaria
force-pushed
the
re-packaging
branch
from
38dbad1
to
a799302
Compare
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-shade-plugin</artifactId> | ||
| <version>3.5.1</version> | ||
| <executions> | ||
| <execution> | ||
| <phase>package</phase> | ||
| <goals> | ||
| <goal>shade</goal> | ||
| </goals> | ||
| <configuration> | ||
| <transformers> | ||
| <transformer implementation="org.apache.maven.plugins.shade.resource.DontIncludeResourceTransformer"> | ||
| <resource>.proto</resource> | ||
| <resource>checkstyle.xml</resource> | ||
| </transformer> | ||
| <transformer implementation="org.apache.maven.plugins.shade.resource.ServicesResourceTransformer"/> | ||
| <transformer implementation="org.apache.maven.plugins.shade.resource.ApacheNoticeResourceTransformer"> | ||
| <addHeader>true</addHeader> | ||
| </transformer> | ||
| </transformers> | ||
| <relocations> | ||
| <relocation> | ||
| <pattern>com.google</pattern> | ||
| <shadedPattern>com.spotify.confidence.shaded.com.google</shadedPattern> | ||
| </relocation> | ||
| <relocation> | ||
| <pattern>io.grpc</pattern> | ||
| <shadedPattern>com.spotify.confidence.shaded.io.grpc</shadedPattern> | ||
| </relocation> | ||
| </relocations> | ||
| <artifactSet> | ||
| <excludes> | ||
| <exclude>dev.openfeature:sdk</exclude> | ||
| <exclude>org.slf4j:slf4j-api</exclude> | ||
| <exclude>javax.annotation:javax.annotation-api</exclude> | ||
| <exclude>org.checkerframework:checker-qual</exclude> | ||
| <exclude>com.google.code.findbugs:jsr305</exclude> | ||
| <exclude>com.google.android:annotations</exclude> | ||
| <exclude>org.codehaus.mojo:animal-sniffer-annotations</exclude> | ||
| <exclude>io.perfmark:perfmark-api</exclude> | ||
| </excludes> | ||
| </artifactSet> | ||
| </configuration> | ||
| </execution> | ||
| </executions> | ||
| </plugin> |
There was a problem hiding this comment.
These needs to be moved to the confidence pom.
There was a problem hiding this comment.
Shading grpc classes from Confidence currently breaks the OpenFeature part, since the latter now doesn't use shaded grpc dependencies. Since the shading is confusing, I wonder if we could remove it until it's clear that we actually need it
There was a problem hiding this comment.
but why would openfeature need to know about grpc?
There was a problem hiding this comment.
This constructor accepting a ManagedChannel from the application and passing it to the Confidence object: link
My understanding is that if we accept the shaded ManagedChannel, the same one should be used in that method's signature, but the shaded version is not visible in the ConfidenceFeatureProvider
No description provided.