-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix: Investigate initializer errors #69
Merged
Merged
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
75fe055
Remove 2 provider setups and include the same setup in no-op manifest
AsimRibo 8c0568a
Revert "Remove 2 provider setups and include the same setup in no-op β¦
KCeh 4b64129
Add check if WorkManager is initialized
KCeh d10925c
Add delegate worker for certificate check
KCeh b2bb4e7
Fix lint
KCeh e80a050
Add annotation to UI thread only method
KCeh f7666a1
Remove redundant manifest tags
KCeh ee6f2e0
Merge branch 'develop' into fix/investigate-initializer-errors
KCeh c023224
Merge branch 'develop' of https://github.com/infinum/android-sentinelβ¦
KCeh 8e5998d
Remove unnecessary work manager initialization
KCeh 337d91f
Typo fix
KCeh fbdf3a9
Replace failure result with exception
KCeh bbd0d8a
Improve DI setup for work manager
KCeh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why is Sentinel removing the work manager initializer? Does it even use anything from the work manager?
I'm worried about this because if the app add sentinel they would "hiddenly" remove the initialization of the work manager. Everything would be fine for the apps using custom work manager initialization, but for the apps using the default one, work manager wouldn't be initialized anymore.
I'd be calmer if we would remove this code completely, therefore not removing the work manager initialization, and therefore leaving the work manager setup as it is defined in the apps.
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.
I will look into that option as well. It should work.
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.
Just to double check. You are suggesting to remove this whole part?
Do you then think we should add this to our README as a reminder for users that they will have to do it in their projects?
I will leave the decision to Karlo as he is the owner.
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.
Yes, I'm suggesting that, but not in case users would need to add this to their projects only because they have Sentinel.
I'm suggesting removing this if it is redundant.
The only thing when users should add this is in case they use a work manager in their project.
Does Sentinel even use a work manager? What for?
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.
I think only usage is in
SentinelWorkManager
class. It is used for certificates checking. Interestingly enough, this is the second time certificates are causing problems π .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.
And I think that is a problem. Since WorkManager can be initialized only once (next time it throws), any application using Work Manager would fail to supply their own workers and initialize the WorkManager. That means, the application dev has to choose, either to use WorkManager and no Sentinel, or vice-versa.
That's the problem we should investigate further and see how to avoid the clash.