refactor!: split the confidence module to a new module #136

vahidlazio · 2024-04-29T14:55:39Z

No description provided.

confidence/build.gradle.kts

nicklasl · 2024-05-02T14:39:13Z

When we merge this we need to include that it is a breaking change.

nicklasl · 2024-05-02T14:41:06Z

release-please-config.json

@@ -10,7 +10,8 @@
            "extra-files": [


I think we should figure out how to make Release please release confidence vs provider independently.

This means splitting into two packages.

We can take this later though

nicklasl · 2024-05-02T14:41:38Z

settings.gradle.kts

@@ -1,5 +1,7 @@
 import org.gradle.api.initialization.resolve.RepositoriesMode

+include(":confidence")
+


rootProject.name in this file needs update.

nicklasl · 2024-05-02T14:42:20Z

confidence/build.gradle.kts

+
+val providerVersion = project.extra["version"].toString()
+
+object Versions {


Ideally we want to keep these versions set in the project gradle file to share them

Are we leaving that to a separate PR?

nicklasl · 2024-05-02T14:51:36Z

ConfidenceDemoApp/src/main/java/com/example/confidencedemoapp/MainVm.kt

            app.applicationContext,
            clientSecret,
+            initialContext = mapOf("targeting_key" to ConfidenceValue.String("a98a4291-53b0-49d9-bae8-73d3f5da2070")),


Would love for us to move away from targeting_key. Do we keep it for the flag rules in the backend expect it?

I'm not sure. i think backend works without targeting key.

yes, the backend doesn't require it, could be that the flag used in this example app requires that to be set though.

nicklasl · 2024-05-02T14:53:24Z

ConfidenceDemoApp/src/main/java/com/example/confidencedemoapp/MainVm.kt

+            if(confidence.isStorageEmpty()) {
+                confidence.fetchAndActivate()
+            } else {
+                confidence.activate()
+                confidence.asyncFetch()
+            }


This signals to me that it would be nice to have an "entry function" for our users to perhaps pass a strategy?

I actually think having control like this is simpler rather than understanding another concept like initializationStrategy, I believe most of the users will only use async fetch or activateAndFetch only.

For a total confidence onboarding I feel this is overkill and would like to have something simpler.

yes, for sure, i would go only with fetchAndActivate.

nicklasl · 2024-05-02T14:56:02Z

confidence/src/main/java/com/spotify/confidence/Confidence.kt

+            }
+        }
+
+    fun activate() {


Is there a time when we do not want to activate?

I think no but confidence object is dumb. I prefer these actions to be explicit and set by the user and be mentioned in the user guide.

nicklasl · 2024-05-07T11:25:30Z

ConfidenceDemoApp/src/main/java/com/example/confidencedemoapp/MainVm.kt

            app.applicationContext,
            clientSecret,
+            initialContext = mapOf("targeting_key" to ConfidenceValue.String("a98a4291-53b0-49d9-bae8-73d3f5da2070")),


yes, the backend doesn't require it, could be that the flag used in this example app requires that to be set though.

nicklasl · 2024-05-07T11:26:25Z

ConfidenceDemoApp/src/main/java/com/example/confidencedemoapp/MainVm.kt

+            if(confidence.isStorageEmpty()) {
+                confidence.fetchAndActivate()
+            } else {
+                confidence.activate()
+                confidence.asyncFetch()
+            }


For a total confidence onboarding I feel this is overkill and would like to have something simpler.

nicklasl · 2024-05-07T11:29:05Z

Provider/src/test/java/com/spotify/confidence/openfeature/InMemorySharedPreferences.kt

@@ -0,0 +1,100 @@
+package com.spotify.confidence.openfeature


This should not be in openfeature.

we also have a integration test in OF which means we are also going to need the same fake.

nicklasl · 2024-05-07T11:29:39Z

confidence/build.gradle.kts

+
+val providerVersion = project.extra["version"].toString()
+
+object Versions {


Are we leaving that to a separate PR?

nicklasl · 2024-05-07T11:30:21Z

confidence/build.gradle.kts

+            pom {
+                name.set("Confidence SDK Android")
+                description.set("Android SDK for Confidence")
+                url.set("https://github.com/spotify/confidence-openfeature-provider-kotlin")


update this and below links to the new repo name.

I open a new PR to address changes regarding the change of repo name.

nicklasl · 2024-05-07T11:31:58Z

confidence/src/main/java/com/spotify/confidence/Confidence.kt

@@ -19,41 +20,69 @@ import kotlinx.coroutines.flow.drop
 import kotlinx.coroutines.launch
 import okhttp3.OkHttpClient

+internal const val SDK_ID = "SDK_ID_KOTLIN_PROVIDER"


do we have a new SDK id for the Confidence SDK?
Also, how do we support both (one for provider and one for stand-alone SDK)?

we don't. provider internally uses the confidence lib, so basically we have confidence. we can however send some metadata for the sdk which can be specified which environment user is using the confidence lib. it can be OF provider etc. wdyt?

We should use SDK_ID_KOTLIN_CONFIDENCE (ref). I agree also on keeping SDK_ID_KOTLIN_PROVIDER if it's used in the OF env

changed to SDK_ID_KOTLIN_CONFIDENCE, however this sdkmetadata is internal to confidence, users from public api shouldn't be able to set if from the outside. we can have a metadata of the environment in which confidence is used to send alongside with the id and version to the backend.

confidence/src/main/java/com/spotify/confidence/Confidence.kt

confidence/src/main/java/com/spotify/confidence/RemoteFlagResolver.kt

confidence/src/main/java/com/spotify/confidence/cache/FileDiskStorage.kt

confidence/build.gradle.kts

ConfidenceDemoApp/build.gradle.kts

Provider/src/main/java/com/spotify/confidence/openfeature/ConfidenceFeatureProvider.kt

vahidlazio · 2024-05-07T14:13:29Z

@fabriziodemaria I added awaitReconciliation function + an extension named awaitPutContext. I think that fixes the issue.

vahidlazio added 3 commits April 29, 2024 16:54

split the confidence module to a new module

3ccd08d

fixup! split the confidence module to a new module

3df4ddc

fixup! fixup! split the confidence module to a new module

b2a7b00

vahidlazio marked this pull request as ready for review April 29, 2024 16:05

vahidlazio requested review from fabriziodemaria, nicklasl and nickybondarenko as code owners April 29, 2024 16:05

vahidlazio added 2 commits April 29, 2024 18:11

provider package is com.spotify.confidence.openfeature

c14df5a

add maven release block for confidence module

210fbdd

nicklasl reviewed May 2, 2024

View reviewed changes

confidence/build.gradle.kts Outdated Show resolved Hide resolved

nicklasl changed the title ~~refactor: split the confidence module to a new module~~ refactor!: split the confidence module to a new module May 2, 2024

nicklasl reviewed May 2, 2024

View reviewed changes

vahidlazio added 6 commits May 7, 2024 11:41

Merge branch 'main' into new-confidence-module

b670062

fixup! Merge branch 'main' into new-confidence-module

09555d6

fixup! fixup! Merge branch 'main' into new-confidence-module

2ff23ee

track activities in mainvm

cdbcb6a

update artifact id

a6ab6a2

fixup! update artifact id

6382907

nicklasl reviewed May 7, 2024

View reviewed changes

vahidlazio added 3 commits May 7, 2024 14:43

change artifact ids to android

2be4afd

update confidence plugin to confidence-android

9514177

make storage and flag resolver internal

b26fa41

fabriziodemaria reviewed May 7, 2024

View reviewed changes

confidence/build.gradle.kts Outdated Show resolved Hide resolved

ConfidenceDemoApp/build.gradle.kts Outdated Show resolved Hide resolved

Provider/src/main/java/com/spotify/confidence/openfeature/ConfidenceFeatureProvider.kt Show resolved Hide resolved

rename confidence to confidence with C

5a69b9b

sdk id should be SDK_ID_KOTLIN_CONFIDENCE

de79655

fabriziodemaria previously approved these changes May 7, 2024

View reviewed changes

introduce await for reconciliation

2180334

vahidlazio dismissed fabriziodemaria’s stale review via 2180334 May 7, 2024 14:12

fabriziodemaria approved these changes May 7, 2024

View reviewed changes

vahidlazio merged commit ebe2733 into main May 7, 2024
2 checks passed

github-actions bot mentioned this pull request May 7, 2024

chore(main): release 0.2.0 #144

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: split the confidence module to a new module #136

refactor!: split the confidence module to a new module #136

vahidlazio commented Apr 29, 2024

nicklasl commented May 2, 2024

nicklasl May 2, 2024

nicklasl May 2, 2024

nicklasl May 2, 2024

nicklasl May 2, 2024

nicklasl May 2, 2024

vahidlazio May 7, 2024

nicklasl May 7, 2024

vahidlazio May 7, 2024

nicklasl May 2, 2024

vahidlazio May 7, 2024

nicklasl May 7, 2024

nicklasl May 2, 2024

vahidlazio May 7, 2024

nicklasl May 7, 2024

vahidlazio May 7, 2024

nicklasl May 2, 2024

vahidlazio May 7, 2024

nicklasl May 7, 2024

nicklasl May 7, 2024

nicklasl May 7, 2024

vahidlazio May 7, 2024

nicklasl May 7, 2024

nicklasl May 7, 2024

vahidlazio May 7, 2024

nicklasl May 7, 2024

vahidlazio May 7, 2024

fabriziodemaria May 7, 2024

vahidlazio May 7, 2024

vahidlazio commented May 7, 2024

		@@ -1,5 +1,7 @@
		import org.gradle.api.initialization.resolve.RepositoriesMode

		include(":confidence")


		val providerVersion = project.extra["version"].toString()

		object Versions {

refactor!: split the confidence module to a new module #136

refactor!: split the confidence module to a new module #136

Conversation

vahidlazio commented Apr 29, 2024

nicklasl commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vahidlazio commented May 7, 2024