Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use 'flowcrypt.com/shared-tenant-fes' instead of 'flowcrypt.com/api' #2171

Merged
merged 62 commits into from
Mar 13, 2023
Merged

Use 'flowcrypt.com/shared-tenant-fes' instead of 'flowcrypt.com/api' #2171

merged 62 commits into from
Mar 13, 2023

Conversation

DenBond7
Copy link
Collaborator

@DenBond7 DenBond7 commented Jan 23, 2023 •
edited by ioanmo226
Loading

This PR added using 'flowcrypt.com/shared-tenant-fes' instead of 'flowcrypt.com/api'

close #2162
close #2138

Tests (delete all except exactly one):

  • Tests added or updated

todo

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities

@DenBond7 DenBond7 requested a review from tomholub January 23, 2023 14:40
@DenBond7 DenBond7 requested a review from tomholub January 25, 2023 09:33
@DenBond7
Copy link
Collaborator Author

@tomholub Please relook the current changes while I'm working on fixing tests.

tomholub
tomholub reviewed Jan 25, 2023
View reviewed changes
FlowCrypt/build.gradle Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/ApiRepository.kt Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/ApiRepository.kt Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/ApiService.kt Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/FlowcryptApiRepository.kt Outdated Show resolved Hide resolved
...pt/src/main/java/com/flowcrypt/email/jetpack/workmanager/HandlePasswordProtectedMsgWorker.kt Outdated Show resolved Hide resolved
...pt/src/main/java/com/flowcrypt/email/jetpack/workmanager/RefreshClientConfigurationWorker.kt Outdated Show resolved Hide resolved
...pt/src/main/java/com/flowcrypt/email/jetpack/workmanager/RefreshClientConfigurationWorker.kt Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/ui/activity/fragment/MainSignInFragment.kt Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/util/GeneralUtil.kt Outdated Show resolved Hide resolved
@DenBond7 DenBond7 requested a review from tomholub January 25, 2023 13:14
@DenBond7
Copy link
Collaborator Author

@tomholub Please relook. I've added the requested changes.

tomholub
tomholub reviewed Jan 26, 2023
View reviewed changes
Copy link
Collaborator

@tomholub tomholub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's improving a lot, see comments

FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/ApiClientRepository.kt Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/ApiClientRepository.kt Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/ApiClientRepository.kt Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/ApiClientRepository.kt Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/ApiClientRepository.kt Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/api/retrofit/RetrofitApiServiceInterface.kt Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/jetpack/viewmodel/ComposeMsgViewModel.kt Outdated Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/ui/activity/fragment/CreateMessageFragment.kt Show resolved Hide resolved
FlowCrypt/src/main/java/com/flowcrypt/email/ui/activity/fragment/MainSignInFragment.kt Outdated Show resolved Hide resolved
@DenBond7 DenBond7 mentioned this pull request Jan 26, 2023
Closed
DenBond7 added 14 commits March 7, 2023 19:00
@DenBond7
Fixed compilation issue.| #2162
57c53fb
@DenBond7
Fixed compilation issue.Step 2.| #2162
9a8527b
@DenBond7
Updated FesDuringSetupEnterpriseFlowTest.| #2162
07084e3
@DenBond7
Refactored code.| #2162
0786af1
@DenBond7
Handled a situation of sign-in users(gmail.com) in the enterprise bui…
b47043d 
…ld.| #2162
@DenBond7
Added EnterpriseTestsFilter.| #2162
e235216
@DenBond7
Update Semaphore settings to run enterprise tests.| #2162
911eaf5
@DenBond7
Update Semaphore settings to run enterprise tests. Step 2.| #2162
15854a5
@DenBond7
Update Semaphore settings to run enterprise tests. Step 3.| #2162
5e468a3
@DenBond7
Update Semaphore settings to run enterprise tests. Step 4.| #2162
3154f56
@DenBond7
Update Semaphore settings to run enterprise tests. Step 5.| #2162
f921e8b
@DenBond7
Marked some tests as FlakyTest.| #2162
505b9b9
@DenBond7
Marked some tests as FlakyTest. Step 2.| #2162
91e91b9
@DenBond7
Merge branch 'master' into issue_2162_shared-tenant-fes
358e803
@DenBond7
Copy link
Collaborator Author

DenBond7 commented Mar 9, 2023

@tomholub I think I've added changes that were requested to migrate to shared-tenant-fes. But all of these changes are related to the setup screen. we still don't support the following:

going forward, client continues attempting to call fes.domain.com first before falling back on flowcrypt.com/shared-tenant-fes anytime it needs to update Client Configuration, and continues using flowcrypt.com/shared-tenant-fes for password protected messages and all other APIs, for as long as fes.domain.com is not available.
if Client Configuration is ever successfully retrieved from fes.domain.com, the app will start using fes.domain.com for all APIs going forward.

it means we decide at the setup screen whether will we use a custom FES server or not. After the setup screen, we don't use a custom FES server if it failed at the setup screen.

If it is possible I propose adding that functionality via a separate PR. The current PR is already not simple.

If yes, I will ask Mart to recheck #2197 and after that, we will be ready to merge it.

@tomholub
Copy link
Collaborator

tomholub commented Mar 9, 2023

yes, the parts after setup can be done in a future version. so ok for @martgil to re check functionality now

@DenBond7 DenBond7 mentioned this pull request Mar 9, 2023
Closed
@DenBond7 DenBond7 mentioned this pull request Mar 13, 2023
Closed
@DenBond7 DenBond7 marked this pull request as ready for review March 13, 2023 09:23
@DenBond7 DenBond7 requested a review from ioanmo226 March 13, 2023 09:24
@DenBond7
Copy link
Collaborator Author

@ioanmo226 Please make the final review.

@ioanmo226
Copy link
Collaborator

ioanmo226 commented Mar 13, 2023 •
edited
Loading

Can I merge this PR with after my review? or waiting some other things?

@DenBond7
Copy link
Collaborator Author

Can I merge this PR with after my review? or waiting some other things?

Yes, we are ready to merge it.

ioanmo226
ioanmo226 approved these changes Mar 13, 2023
View reviewed changes
Copy link
Collaborator

@ioanmo226 ioanmo226 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ioanmo226 ioanmo226 merged commit ad44f9d into master Mar 13, 2023
@ioanmo226 ioanmo226 deleted the issue_2162_shared-tenant-fes branch March 13, 2023 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ioanmo226 ioanmo226 ioanmo226 approved these changes

@tomholub tomholub tomholub approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@DenBond7 @tomholub @ioanmo226