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.

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

refactor: add base path in Operate and Tasklist apps urls #18604

Merged
merged 39 commits into from
Jun 5, 2024

Conversation

houssain-barouni
Copy link
Collaborator

@houssain-barouni houssain-barouni commented May 16, 2024

Description

The Camunda single application should serve Zeebe, Operate and Tasklist using one running application.

To dispatch between Operate and Tasklist UIs, we need to add /operate and /tasklist base paths to the URLs respectively used by Operate and Tasklist UIs
For consistency, the same base paths will be used when Operate and Tasklist are run in standalone mode.

  • requests to /tasklist and /operate are redirected to Tasklist or Operate index file
  • requests to the root domain are redirect to either /tasklist or /operate depending on the used profile in standalone mode, it defaults to /operate in single application mode.
  • backend API urls do not change and do not contain the base paths.
  • old app URLs not containing the base path (example http://localhost:8080/processes) will not be working, the app base path should be provided in the URL (example http://localhost:8080/tasklist/processes)
  • Tasklist public forms URLs (/new/{bpmnProcessId}do not require the base path and will continue to be working as they are served by a dedicated endpoint in backend.

Related issues

relates to #18535

@github-actions github-actions bot added component/zeebe Related to the Zeebe component/team component/operate Related to the Operate component/team component/tasklist Related to the Tasklist component/team labels May 16, 2024
Copy link
Contributor

github-actions bot commented May 16, 2024

Operate Test Results

132 files  ±0  132 suites  ±0   8m 15s ⏱️ +25s
798 tests ±0  793 ✅  - 2  3 💤 ±0  2 ❌ +2 
797 runs   - 1  792 ✅  - 3  3 💤 ±0  2 ❌ +2 

For more details on these failures, see this check.

Results for commit 3b1ed0a. ± Comparison against base commit 01c1855.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented May 16, 2024

Tasklist Test Results

143 files  ± 0  143 suites  ±0   1h 27m 2s ⏱️ - 16m 6s
551 tests + 2  546 ✅ + 2  5 💤 ±0  0 ❌ ±0 
517 runs   - 30  512 ✅  - 30  5 💤 ±0  0 ❌ ±0 

Results for commit d6e6238. ± Comparison against base commit 0283ee6.

This pull request removes 13 and adds 15 tests. Note that renamed tests count towards both.
io.camunda.tasklist.webapp.api.rest.v1.controllers.external.ProcessExternalControllerTest ‑ startProcessWithEmbeddedForm(HttpStatus, String, ProcessEntity, ProcessInstanceDTO)[1] expectedHttpStatus=200 OK, bpmnProcessId=StartProcessByForm, processEntity=TenantAwareTasklistEntity[id='1', tenantId='<default>'], providedProcessInstanceDTO=io.camunda.tasklist.webapp.graphql.entity.ProcessInstanceDTO@9368393b
io.camunda.tasklist.webapp.api.rest.v1.controllers.external.ProcessExternalControllerTest ‑ startProcessWithLinkedForm(HttpStatus, String, ProcessEntity, ProcessInstanceDTO)[1] expectedHttpStatus=200 OK, bpmnProcessId=StartProcessByForm, processEntity=TenantAwareTasklistEntity[id='1', tenantId='<default>'], providedProcessInstanceDTO=io.camunda.tasklist.webapp.graphql.entity.ProcessInstanceDTO@872afa3f
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginAndUsageOfGraphQlApi(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007bafc5317db8@1a300b53
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginAndUsageOfRestApi(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007bafc5317db8@1a300b53
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginFailedWithNoPermissions(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007bafc5317db8@1a300b53
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginFailedWithNoReadPermissions(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007bafc5317db8@1a300b53
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginSucceedWithNoWritePermissions(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007bafc5317db8@1a300b53
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginSuccess(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007bafc5317db8@1a300b53
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLogout(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007bafc5317db8@1a300b53
io.camunda.tasklist.webapp.security.sso.AuthenticationWithPersistentSessionsIT ‑ testLoginFailedWithNoPermissions(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationWithPersistentSessionsIT$$Lambda/0x00007945ad35eb40@586bc8ed
…
io.camunda.tasklist.modules.OnlyArchiverIT ‑ testTasklistIndexControllerIsNotPresent
io.camunda.tasklist.modules.OnlyImportIT ‑ testTasklistIndexControllerIsNotPresent
io.camunda.tasklist.webapp.api.rest.v1.controllers.external.ProcessExternalControllerTest ‑ startProcessWithEmbeddedForm(HttpStatus, String, ProcessEntity, ProcessInstanceDTO)[1] expectedHttpStatus=200 OK, bpmnProcessId=StartProcessByForm, processEntity=TenantAwareTasklistEntity[id='1', tenantId='<default>'], providedProcessInstanceDTO=io.camunda.tasklist.webapp.graphql.entity.ProcessInstanceDTO@bab23d2
io.camunda.tasklist.webapp.api.rest.v1.controllers.external.ProcessExternalControllerTest ‑ startProcessWithLinkedForm(HttpStatus, String, ProcessEntity, ProcessInstanceDTO)[1] expectedHttpStatus=200 OK, bpmnProcessId=StartProcessByForm, processEntity=TenantAwareTasklistEntity[id='1', tenantId='<default>'], providedProcessInstanceDTO=io.camunda.tasklist.webapp.graphql.entity.ProcessInstanceDTO@6186d546
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginAndUsageOfGraphQlApi(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007dff5d331858@52833d4b
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginAndUsageOfRestApi(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007dff5d331858@52833d4b
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginFailedWithNoPermissions(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007dff5d331858@52833d4b
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginFailedWithNoReadPermissions(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007dff5d331858@52833d4b
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginSucceedWithNoWritePermissions(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007dff5d331858@52833d4b
io.camunda.tasklist.webapp.security.sso.AuthenticationIT ‑ testLoginSuccess(BiFunction)[1] orgExtractor=io.camunda.tasklist.webapp.security.sso.AuthenticationIT$$Lambda/0x00007dff5d331858@52833d4b
…

♻️ This comment has been updated with latest results.

@houssain-barouni houssain-barouni force-pushed the commom_index_controller branch 4 times, most recently from 01a7763 to f4e79f3 Compare May 16, 2024 21:21
Copy link
Contributor

github-actions bot commented May 17, 2024

Operate Unit Tests Results

299 tests  ±0   298 ✅ ±0   6m 28s ⏱️ +28s
 45 suites ±0     1 💤 ±0 
 45 files   ±0     0 ❌ ±0 

Results for commit d6e6238. ± Comparison against base commit 0283ee6.

♻️ This comment has been updated with latest results.

Base automatically changed from move_tasklist_application to main May 17, 2024 14:31
Copy link
Contributor

github-actions bot commented May 17, 2024

Operate Integration Tests Results

525 tests   523 ✅  7m 57s ⏱️
 88 suites    2 💤
 88 files      0 ❌

Results for commit d6e6238.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented May 17, 2024

Operate Opensearch ITs Results

182 tests  ±0   182 ✅ ±0   1m 8s ⏱️ +7s
 19 suites ±0     0 💤 ±0 
 19 files   ±0     0 ❌ ±0 

Results for commit d6e6238. ± Comparison against base commit 0283ee6.

♻️ This comment has been updated with latest results.

@houssain-barouni houssain-barouni force-pushed the commom_index_controller branch 2 times, most recently from 577b40f to 7907345 Compare May 17, 2024 20:25
@houssain-barouni houssain-barouni changed the base branch from main to tasklist_properties May 21, 2024 09:18
Base automatically changed from tasklist_properties to main May 21, 2024 12:37
@houssain-barouni
Copy link
Collaborator Author

For transparency, a discussion with @vsgoulart about public-forms E2E test that always fail the first time on this branch, but always passes on main:

On main branch:

  • the process is not found in the first hit when /new/{processId} .you can also notice the title is missing
  • even though the process is not found, the backend returns 200 and forwards to index.html
  • the reason is that:
    • PublicProcessController does not extend ApiErrorController , so all the exceptions are transformed into 500
    • we had an ErrorController that catches all 500 responses and forwards to index.html with 200 status
  • the UI then fetches the form, which can be not found in the first hit, but the UI does retries to fetch the form
    this ends-up by working after some retries

On this branch, as the ErrorController was removed, the backend returns 500 when the process is not found

To fix this:

  • PublicProcessController should extend ApiErrorController so that 404 is returned when process is not found
  • then we have two options:
    • Make the UI handle the 404 returned /new/{processId} , do some retries, and display the error-robot if the process cannot be found
    • catch the 404 in backend and returnindex.htmlwith 200 status, the UI will fail to fetch the form if the process does not really exist (as before)

Copy link
Contributor

@pedesen pedesen left a comment

Choose a reason for hiding this comment

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

@houssain-barouni There is an issue with the local dev environment the Operate FE team is using.

  • if you run yarn start in operate/client/, Operate provides a hot reload dev environment running at localhost:3000
  • with your changes /client-config.js can't be retrieved anymore in that dev environment

I think the reason for this is that we use a middleware (see setupProxy.js) to redirect localhost:3000/client-config.js to localhost:8080/client-config.js. But now it needs to be redirected to localhost:8080/operate/client-config.ts

So you would either move client-config.ts or redirect to localhost:8080/operate/client-config.ts

@houssain-barouni
Copy link
Collaborator Author

houssain-barouni commented Jun 4, 2024

@houssain-barouni There is an issue with the local dev environment the Operate FE team is using.

  • if you run yarn start in operate/client/, Operate provides a hot reload dev environment running at localhost:3000
  • with your changes /client-config.js can't be retrieved anymore in that dev environment

I think the reason for this is that we use a middleware (see setupProxy.js) to redirect localhost:3000/client-config.js to localhost:8080/client-config.js. But now it needs to be redirected to localhost:8080/operate/client-config.ts

So you would either move client-config.ts or redirect to localhost:8080/operate/client-config.ts

I added the base path in setupProxy.js , and tested that is working with yarn start
I also added -DskipQaBuild in makefile build commands. QA modules depend on dist so it can fail if /dist contain changes and is not built beforehand. QA modules are not needed anyway to start the application

operate/client/src/setupProxy.js Outdated Show resolved Hide resolved
@pedesen pedesen self-requested a review June 5, 2024 09:59
Copy link
Contributor

@pedesen pedesen left a comment

Choose a reason for hiding this comment

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

I checked that everything works fine. There is just one change needed in the playwright.config (see my comment there).

if (baseURL && baseURL.endsWith('/')) {
baseURL = baseURL.slice(0, -1);
}

// Important: make sure we authenticate in a clean environment by unsetting storage state.
const page = await browser.newPage({storageState: undefined});
await page.goto(`${baseURL}/login`);
Copy link
Contributor

@pedesen pedesen Jun 5, 2024

Choose a reason for hiding this comment

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

🔧 Maybe you can utilize URL here instead of manually stripping the trailing slash. This should also remove any double slashes:

new URL('/login', baseURL).toString()

Copy link
Collaborator Author

@houssain-barouni houssain-barouni Jun 5, 2024

Choose a reason for hiding this comment

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

Hi @pedesen
new URL('/login', baseURL).toString() uses absolute path and would return http://localhost:8080/login instead of http://localhost:8080/operate/login
I can use absolute path new URL('/operate/login', baseURL).toString() but that would make the other CIs failing like a11y

Copy link
Contributor

Choose a reason for hiding this comment

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

For transparency: We discussed to leave it as it is, because it was not possible to use new URL without breaking other things.

operate/client/playwright.config.ts Outdated Show resolved Hide resolved
@pedesen pedesen self-requested a review June 5, 2024 14:52
@houssain-barouni houssain-barouni added this pull request to the merge queue Jun 5, 2024
Merged via the queue into main with commit a7af48f Jun 5, 2024
66 of 67 checks passed
@houssain-barouni houssain-barouni deleted the commom_index_controller branch June 5, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/operate Related to the Operate component/team component/tasklist Related to the Tasklist component/team component/zeebe Related to the Zeebe component/team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants