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

[JBPM-9464] Kie server notification causes bad status in BC #2327

Closed
wants to merge 1 commit into from
Closed

[JBPM-9464] Kie server notification causes bad status in BC #2327

wants to merge 1 commit into from

Conversation

elguardian
Copy link
Member

@elguardian elguardian commented Nov 27, 2020 •
edited
Loading

@elguardian elguardian force-pushed the JBPM-9464 branch 2 times, most recently from 2765f5e to df5bb35 Compare November 30, 2020 14:55
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

64.1% 64.1% Coverage
3.4% 3.4% Duplication

@elguardian elguardian requested a review from gmunozfe December 11, 2020 07:58
@elguardian elguardian mentioned this pull request Dec 15, 2020
Closed
@elguardian
Copy link
Member Author

Jenkins retest this please

@elguardian
Copy link
Member Author

Jenkins retest this

@elguardian
Copy link
Member Author

Jenkins retest this

1 similar comment
@elguardian
Copy link
Member Author

Jenkins retest this

@elguardian
Copy link
Member Author

Jenkins retest this

@gmunozfe
Copy link
Member

jenkins retest this

gmunozfe
gmunozfe reviewed Jan 18, 2021
View reviewed changes
Copy link
Member

@gmunozfe gmunozfe left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me, but need to fix failing test (KieServerInstanceManagerTest.testGetContainersRemoteOperationWhenResponseTypeIsSUCCESS),
by adding the expected status to the mocked container:
doReturn(KieContainerStatus.STARTED).when(container).getStatus();

Some other minor comments

...r-controller-impl/src/main/java/org/kie/server/controller/impl/KieServerInstanceManager.java Outdated Show resolved Hide resolved
...-common/src/main/java/org/kie/server/services/impl/controller/DefaultRestControllerImpl.java Outdated Show resolved Hide resolved
...-common/src/main/java/org/kie/server/services/impl/controller/DefaultRestControllerImpl.java Show resolved Hide resolved
@gmunozfe gmunozfe self-requested a review January 18, 2021 14:26
@elguardian elguardian requested a review from sutaakar January 21, 2021 08:17
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

61.0% 61.0% Coverage
3.8% 3.8% Duplication

@sutaakar
Copy link
Contributor

Checked with Kie server and standalone controller - seems to work ok.
Will check with workbench once FDB on linked PR finish (so I can download the built workbench).

@sutaakar
Copy link
Contributor

sutaakar commented Feb 4, 2021

@elguardian When I use WebSockets for connection between Kie server and Business central then I get this warning:

13:47:39,063 WARN  [org.kie.server.services.impl.controller.DefaultRestControllerImpl] (Thread-187) Exception encountered while syncing with controller at ws://localhost:8080/business-central/websocket/controller/server/default-kieserver error unknown protocol: ws

The warning is shown for example when container is started or stopped using Business central.
Apart from this it seems to work well.

@elguardian elguardian closed this Jun 18, 2021
@elguardian elguardian deleted the JBPM-9464 branch June 18, 2021 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmunozfe gmunozfe Awaiting requested review from gmunozfe

@sutaakar sutaakar Awaiting requested review from sutaakar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@elguardian @gmunozfe @sutaakar