dynamic cockpit link resolution via well known endpoint #4297

AlexanderSkrock · 2024-05-18T19:19:09Z

Aims to fix: #4261

Requirements for this functionality were discussed here: #4261 (comment)

AlexanderSkrock · 2024-05-21T23:20:53Z

Open tasks

write tests for the link resolution logic
fix tests for both tool components, i expect failure because of the asynchronous call to set the link
update/extend documentation

nikku · 2024-06-13T12:54:20Z

@AlexanderSkrock Any blockers that you encountered working on this solution? We're happy to assist where needed.

AlexanderSkrock · 2024-06-16T00:07:02Z

Hi @nikku ,

thank you for asking! I appreciate your assistance as always! Right now, there are no blockers except that I currently have no spare time due to private circumstances. Tho, I think I'll be able to follow up in a couple weeks. As this did not appear to be a priority topic, I did not give a sign myself.

Kind regards
Adagatiya

AlexanderSkrock · 2024-07-11T11:20:00Z

Hi @nikku ,

finally, I was able to come back to this topic!

From the implementation side, I would say it is complete and working. I would like to extend the documentation now, but I am not quite sure where to put it. I was able to locate the Camunda Desktop Modeler documentation within the camunda-docs repo . I could add it below the start instance picture here, but it would not be as obvious for someone specifically looking for such a feature.

Do you have any ideas? Maybe I overlooked a section.

Kind regards
Adagatiya

barmac · 2024-08-02T15:46:36Z

Hi @AlexanderSkrock,

Thanks for your contribution, and sorry for a delay in review. I noticed that the PR is failing on CI due to a linting problem. Can you please fix it so that we can verify the tests pass?

AlexanderSkrock · 2024-08-02T16:36:19Z

Hi @barmac ,

thank you for the response! I was on vacation the previous week anyways.
I am not sure how I overlooked that linting error, sorry for that!

Did you see my question / thoughts about documentation changes?

Kind regards
Adagatiya

AlexanderSkrock · 2024-08-11T16:25:35Z

I rebased my branch once again onto the current development branch, so feel free to review now, @barmac . Also, all tests passed on my fork's actions.

barmac · 2024-08-12T08:33:04Z

I am going to look into this today.

barmac

Hi, this PR is bigger than what I'd expect because of the refactorings included. Please have a look at the comments that I left, and hopefully we should be able to merge this soon. Thanks again for your contribution.

barmac · 2024-08-12T14:31:56Z

client/src/plugins/camunda-plugin/shared/WellKnownAPI.js

+  async getAdminUrl() {
+    const {
+      admin
+    } = await this.getWellKnownWebAppUrls();


If I understand it correctly, we request the well known API each time we try to check each of the application's url. Is this necessary?

Yes, this is correct. Of course, we could cache the response, but then again, we need to think about when to clear the cache. Because it should not change except for development purposes I could think of a internal map structure within the WellKnownApi file. This "cache" would be evicted on a reload via dev tools or on restarting the Camunda Desktop Modeler.

How important would caching be for you? And if it was preferred, would cache evict on restart be sufficient?

I think it's OK if it does not slow down the flow too much. The deployment does not happen all the time anyway...

barmac · 2024-08-12T14:34:02Z

client/src/plugins/camunda-plugin/deployment-tool/__tests__/DeploymentConfigOverlaySpec.js

-      instance.isOnBeforeSubmit = true;
-
-      wrapper.update();
+      setTimeout(() => {


I understand that the current test suite for the deployment tool is in bad shape, but I'd rather expect us to properly make use of act and waitFor instead of introducing more setTimeout calls. Let's not make the tests depend on the time.

I am not sure about the Formik workaround with the isOnBeforeSubmit flag which seems to stop working as soon as I replace setTimeout.

I managed to get this working now, but I had to introduce a new dependency @testing-library/react so we have those calls available.

client/src/plugins/camunda-plugin/shared/ui/CockpitProcessInstanceLink.js

client/src/plugins/camunda-plugin/shared/ui/CockpitDeploymentLink.js

barmac · 2024-08-12T14:37:39Z

client/src/plugins/camunda-plugin/shared/webAppUrls.js

+    const cockpitUrl = await forEngineRestUrl(engineUrl).getCockpitUrl();
+
+    if (cockpitUrl) {
+      console.debug(`Using cockpit url from well known endpoint: ${engineUrl}`);


I'd rather use debug dependency consistently instead of console.debug which is the same as console.log.

I exchanged those calls.

barmac · 2024-08-12T14:40:00Z

Please don't mind the failing CI job for ubuntu/pull_request. This is happening because a community PR does not have access to the secrets.

AlexanderSkrock · 2024-08-12T14:48:50Z

Hi, this PR is bigger than what I'd expect because of the refactorings included. Please have a look at the comments that I left, and hopefully we should be able to merge this soon. Thanks again for your contribution.

Hi @barmac ,
should I try to minimize the changes? Then I'd revert the refactorings.

AlexanderSkrock · 2024-08-31T18:38:16Z

Hi @barmac ,

could you please have a look on the changes / my answers to the open discussion again? It's been some time and I'd personally love to know whether there are any open tasks for me.

Kind regards
Alexander Skrock

barmac · 2024-09-17T07:17:22Z

Sorry, I was off when you wrote this. I will have a look this week.

client/src/plugins/camunda-plugin/deployment-tool/__tests__/DeploymentConfigOverlaySpec.js

barmac · 2024-09-20T13:19:16Z

Unfortunately, this is now failing on the CI. Can you please have a look @AlexanderSkrock?

AlexanderSkrock · 2024-09-20T19:02:21Z

Good catch about the missing await keywords!

Unfortunately, this is now failing on the CI. Can you please have a look @AlexanderSkrock?

I am not entirely sure about the reasons, but the submit simulation was wrapped inside setTimeout before as well with a comment about it. I reverted it back to use setTimeout for the submit. I think this setTimeout call should be fine, because we do not wait static 200ms afterwards anymore, but instead make use of the waitFor utility.

Kind regards
Adagatiya

barmac · 2024-09-23T07:31:35Z

Thanks, let's check if it works again on CI. I am aware that we may experience some flakiness in the deployment component, but it should not always fail until we fix it.

barmac · 2024-09-23T09:25:27Z

OK it works (ignore the import secrets failure). We will merge this within a couple of days as we are now in the code freeze. Thank you for your PR!

The main problems before were stubbing away the well known api dependency as well as shallow rendering for the notification.

Co-authored-by: Maciej Barelkowski <maciejbarel@gmail.com>

barmac · 2024-09-30T13:43:33Z

I just squashed and merged this contribution. Thank you @AlexanderSkrock!

AlexanderSkrock force-pushed the 4261 branch from 92b019e to f2e5709 Compare May 18, 2024 20:57

AlexanderSkrock force-pushed the 4261 branch from 3359aad to 3d8f700 Compare July 12, 2024 05:42

AlexanderSkrock marked this pull request as ready for review July 12, 2024 08:06

AlexanderSkrock force-pushed the 4261 branch 3 times, most recently from cf5293a to eaf2653 Compare July 12, 2024 16:08

barmac self-requested a review July 16, 2024 09:33

AlexanderSkrock force-pushed the 4261 branch 2 times, most recently from 8c6c3ca to 344b6c3 Compare August 11, 2024 16:16

barmac reviewed Aug 12, 2024

View reviewed changes

AlexanderSkrock force-pushed the 4261 branch from 385a152 to 68209ff Compare August 22, 2024 19:50

barmac self-requested a review September 17, 2024 07:17

barmac added this to the M81 milestone Sep 17, 2024

barmac force-pushed the 4261 branch from 68209ff to 300792e Compare September 17, 2024 07:17

barmac added the needs review Review pending label Sep 17, 2024 — with bpmn-io-tasks

nikku modified the milestones: M81, M82 Sep 20, 2024

barmac reviewed Sep 20, 2024

View reviewed changes

client/src/plugins/camunda-plugin/deployment-tool/__tests__/DeploymentConfigOverlaySpec.js Outdated Show resolved Hide resolved

AlexanderSkrock force-pushed the 4261 branch from bf24fb5 to d1fc373 Compare September 20, 2024 19:02

barmac force-pushed the 4261 branch from d1fc373 to c0c704b Compare September 23, 2024 07:30

AlexanderSkrock and others added 20 commits September 30, 2024 15:09

chore(cockpit-links) refactor into a common react

2eeae23

feat(well-known-api) dynamic webapp url resolution

55e9e93

fix(cockpit-links): issues with test cases

3a884db

The main problems before were stubbing away the well known api dependency as well as shallow rendering for the notification.

chore(cockpit-links): additional test cases for default handling

9d27ce9

fix(wellknown-api): aligned field names

e4be694

chore(rest-api): extracted duplicate code

53f03b3

chore(cockpit-link): fix linting

81d896f

test(client): use constants for status codes

15d942a

chore(validator): fix linting

5788cba

chore(tests) align test case with basic auth valiation

3b47010

chore(cockpit-link): remove unnecessary useMemo usage

80101e4

Co-authored-by: Maciej Barelkowski <maciejbarel@gmail.com>

chore(cockpit-link): remove unnecessary useMemo usage

4442452

Co-authored-by: Maciej Barelkowski <maciejbarel@gmail.com>

chore(logging): use debug library instead of console.debug

a08e5cb

chore(cockpit-link): remove unnecessary useMemo usage

39a42a3

chore(cockpit-link): remove unnecessary useMemo usage

ddc6529

chore(tests): migrate single testcase to react testing library

8a2a0b9

chore(tests): migrate the remaining testcases to react testing library

165e746

test: apply suggestions from code review

57cd89f

test: fix syntax errors

92ef9a4

tests(deployment-overlay): fix submit simulation

9f3108e

barmac force-pushed the 4261 branch from c0c704b to 9f3108e Compare September 30, 2024 13:09

barmac merged commit 6ff1d3e into camunda:develop Sep 30, 2024
6 of 7 checks passed

bpmn-io-tasks bot removed the needs review Review pending label Sep 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dynamic cockpit link resolution via well known endpoint #4297

dynamic cockpit link resolution via well known endpoint #4297

AlexanderSkrock commented May 18, 2024 •

edited

Loading

AlexanderSkrock commented May 21, 2024 •

edited

Loading

nikku commented Jun 13, 2024

AlexanderSkrock commented Jun 16, 2024 •

edited

Loading

AlexanderSkrock commented Jul 11, 2024

barmac commented Aug 2, 2024

AlexanderSkrock commented Aug 2, 2024 •

edited

Loading

AlexanderSkrock commented Aug 11, 2024

barmac commented Aug 12, 2024

barmac left a comment

barmac Aug 12, 2024

AlexanderSkrock Aug 12, 2024 •

edited

Loading

barmac Sep 20, 2024

barmac Aug 12, 2024

AlexanderSkrock Aug 12, 2024 •

edited

Loading

AlexanderSkrock Aug 22, 2024 •

edited

Loading

barmac Aug 12, 2024

AlexanderSkrock Aug 12, 2024

barmac commented Aug 12, 2024

AlexanderSkrock commented Aug 12, 2024

AlexanderSkrock commented Aug 31, 2024

barmac commented Sep 17, 2024

barmac commented Sep 20, 2024

AlexanderSkrock commented Sep 20, 2024 •

edited

Loading

barmac commented Sep 23, 2024

barmac commented Sep 23, 2024

barmac commented Sep 30, 2024

dynamic cockpit link resolution via well known endpoint #4297

dynamic cockpit link resolution via well known endpoint #4297

Conversation

AlexanderSkrock commented May 18, 2024 • edited Loading

AlexanderSkrock commented May 21, 2024 • edited Loading

nikku commented Jun 13, 2024

AlexanderSkrock commented Jun 16, 2024 • edited Loading

AlexanderSkrock commented Jul 11, 2024

barmac commented Aug 2, 2024

AlexanderSkrock commented Aug 2, 2024 • edited Loading

AlexanderSkrock commented Aug 11, 2024

barmac commented Aug 12, 2024

barmac left a comment

Choose a reason for hiding this comment

barmac Aug 12, 2024

Choose a reason for hiding this comment

AlexanderSkrock Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

barmac Sep 20, 2024

Choose a reason for hiding this comment

barmac Aug 12, 2024

Choose a reason for hiding this comment

AlexanderSkrock Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

AlexanderSkrock Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

barmac Aug 12, 2024

Choose a reason for hiding this comment

AlexanderSkrock Aug 12, 2024

Choose a reason for hiding this comment

barmac commented Aug 12, 2024

AlexanderSkrock commented Aug 12, 2024

AlexanderSkrock commented Aug 31, 2024

barmac commented Sep 17, 2024

barmac commented Sep 20, 2024

AlexanderSkrock commented Sep 20, 2024 • edited Loading

barmac commented Sep 23, 2024

barmac commented Sep 23, 2024

barmac commented Sep 30, 2024

AlexanderSkrock commented May 18, 2024 •

edited

Loading

AlexanderSkrock commented May 21, 2024 •

edited

Loading

AlexanderSkrock commented Jun 16, 2024 •

edited

Loading

AlexanderSkrock commented Aug 2, 2024 •

edited

Loading

AlexanderSkrock Aug 12, 2024 •

edited

Loading

AlexanderSkrock Aug 12, 2024 •

edited

Loading

AlexanderSkrock Aug 22, 2024 •

edited

Loading

AlexanderSkrock commented Sep 20, 2024 •

edited

Loading