-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Merge release/20.9 after 20.9-rc-3 #17320
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
Conversation
…shared-login-provider [Fix] [Shared login] Add feature flag to SharedLoginProvider
| sortOrder: String? | ||
| ): Cursor? { | ||
| inject() | ||
| if (!jetpackProviderSyncFeatureConfig.isEnabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, after solving the conflict like I explained in the PR description (see 6907721) — which got rid of the import org.wordpress.android.util.config.JetpackProviderSyncFeatureConfig — that line no longer compiles and makes CI goes red with:
e: /var/lib/buildkite-agent/builds/ci-android-i-00464ed7936fef979-1/automattic/wordpress-android/WordPress/src/main/java/org/wordpress/android/sharedlogin/provider/SharedLoginProvider.kt: (31, 14): Unresolved reference: jetpackProviderSyncFeatureConfig
Which means that my conflict resolution was probably wrong after all.
@RenanLukas can I let you fix this and redo the conflict resolution properly, since you're the one who worked on those lines of code? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @AliSoftware 👋
Thanks for looking into this conflict. ClientVerification replaces what we've been doing with JetpackPublicData, so removing JetpackPublicData is correct.
Looks like what's missing in SharedLoginProvider is the JetpackProviderSyncFeatureConfig import and also the injected variable, so basically this line with the other imports:
import org.wordpress.android.util.config.JetpackProviderSyncFeatureConfig
And this line with the other injected variables:
@Inject lateinit var jetpackProviderSyncFeatureConfig: JetpackProviderSyncFeatureConfig
How should we proceed in this branch? Should I submit a PR targeting this branch adding these two lines?
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RenanLukas thanks for taking a look!
Feel free to directly push a new commit to fix the issue directly on this PR. This PR is already using merge/20.9-rc-3 as the head branch (and not release/20.9) as I already created and cut that merge branch from the release branch exactly in order to solve the conflict within that intermediate branch without risking impacting the release branch itself while solving the conflict.
So everything is already set for you to continue push more commits on that merge/20.9-rc-3 branch, to finish solving the conflict resolution properly; no need for an separate PR 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 💯
Release wise, everything looks good, but I'll let @RenanLukas and/or @develric approve this PR, that is, after they verify that the conflict resolution is the expected one (plus this).
|
|||||||||||
| 💡 Scan this QR code with your Android phone to download and install the APK directly on it. | ||
| App | WordPress | |
| Build Flavor | Jalapeno | |
| Build Type | Debug | |
| Commit | 076befb | |
|
|||||||||||
| 💡 Scan this QR code with your Android phone to download and install the APK directly on it. | ||
| App | Jetpack | |
| Build Flavor | Jalapeno | |
| Build Type | Debug | |
| Commit | 076befb | |
|
@ParaskP7 @RenanLukas can i have a formal approval of the PR so that it can finally be merged? 🙇🏻 |


Merging the release branch after doing a new beta.
The new beta includes:
strings.xmlfrom GlotPressversion.propertiesConflicts Resolution
On
WordPress/src/main/java/org/wordpress/android/sharedlogin/provider/SharedLoginProvider.kt:I solved it via 6907721 by picking the version of the code from
trunk, i.e. the one using theClientVerificationclass (instead of the twoJetpack*ones).Cc @RenanLukas (or @develric ?) to validate that this was the correct conflict resolution that we want to keep in
trunkonce this PR lands 🙏