Modularisation Part1 #1954

lucidd · 2020-02-25T14:54:01Z

What does this PR change?

This PR contains the first batch of changes for modernizing our codebase better. This PR contains:

introducing an interface for all salt (non ssh) operations.
removing some low level salt calls with higher level methods.
marking future methods to be refined as deprecated.
first batch of salt dependent classes changed to have non static method and take dependencies though constructor.
pushing instantiation of of components to the entry points (xml rpc, router, salt reactor).
no logic should have been changed during this.
TODO: double check synchronized methods since multiple instances will change behavior here.

GUI diff

No difference.

DONE

Documentation

No documentation needed: no user visible changes
DONE

Test coverage

No tests: covered by existing tests
DONE

Links

Fixes #
Tracks # add downstream PR, if any

DONE

Changelogs

If you don't need a changelog check, please mark this checkbox:

No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

renner

I have been deploying this patch and running some tests on a local environment, everything good so far! 👍

java/code/src/com/suse/manager/webui/services/impl/SystemQuery.java

java/code/src/com/suse/manager/webui/services/SaltServerActionService.java

java/code/src/com/suse/manager/webui/services/impl/SystemQuery.java

admd

Thank you @lucidd for the effort. I like the direction we are going in. I have just left some small nitpicks, the rest looks good.

java/code/src/com/redhat/rhn/common/messaging/MessageQueue.java

java/code/src/com/redhat/rhn/frontend/xmlrpc/HandlerFactory.java

java/code/src/com/redhat/rhn/frontend/xmlrpc/api/ApiHandler.java

java/code/src/com/redhat/rhn/manager/formula/FormulaManager.java

java/code/src/com/suse/manager/reactor/messaging/MinionStartEventMessageAction.java

java/code/src/com/suse/manager/reactor/messaging/RegisterMinionEventMessageAction.java

java/code/src/com/suse/manager/webui/services/iface/SystemQuery.java

moio

Hey @lucidd, thanks for all the work here, I think it goes towards the right direction.

I left the bulk of comments inline, many are just nitpicks.

Final notes here: please review this PR's description, it seems like some sections were just marked DONE with no changes in the text (eg. "Test coverage" - I do not really see new Cucumber tests). In particular:

what is the intention when it comes to backporting this to downstream branches, is there any?
is there an intention to run the Cucumber testsuite against this branch before merging?

Look forward to see this merged soon.

moio · 2020-03-15T15:25:34Z

java/code/src/com/redhat/rhn/frontend/xmlrpc/HandlerFactory.java

+     * HandlerFactory prepopulated with unit test handlers.
+     * @return HandlerFactory for unit tests.
+     */
+    public static HandlerFactory mockHandlers() {


Nitpicks:

method name: as this returns a Factory (rather than a Collection of Handlers), shouldn't these be named something like getTestHandlerFactory/getDefaultHandlerFactory instead of mockHandlers/defaultHandlers?

shouldn't mockHandlers ideally live in unit test code only? Are there cons with that?

I'm not really happy with the naming overall here after the refactoring but i did not want to make too many changes in this PR. I adopted the names you suggest for now but i guess a follow up PR can just focus on finding more fitting names for those classes.

Also moved the mocking to a test utils class.

java/code/src/com/redhat/rhn/frontend/xmlrpc/HandlerFactory.java

java/code/src/com/redhat/rhn/frontend/xmlrpc/system/test/SystemHandlerTest.java

moio · 2020-03-15T16:03:22Z

java/code/src/com/redhat/rhn/frontend/xmlrpc/HandlerFactory.java

-    public BaseHandler getHandler(String handlerName) {
-        return (BaseHandler)factory.getObject(handlerName);
+    public Optional<BaseHandler> getHandler(String handlerName) {
+        return Optional.ofNullable(handlers.get(handlerName));


I wonder if returning Optional is the right choice here, as I see that basically all call sites immediately use get() on the result.

How about instead throwing an unchecked, descriptive Exception here that will result in something like "handlerName not found" in logs, instead of NPE, whenever handlers.get yields null - thereby avoiding the Optional wrapping?

What do you think, what would be pros and cons?

looking at the invocations there is really only 1 place that looks up a handler by user input. I added a orElseThrow there with a descriptive message. All the other invocations are either in tests or by using only values from getKeys. Optimally we should probably change this method to return a Set<Tuple2<String, BaseHandler>> so there is no need for an unsafe get.

java/code/src/com/suse/manager/webui/services/iface/SystemQuery.java

moio · 2020-03-15T16:45:02Z

java/code/src/com/redhat/rhn/manager/system/entitling/SystemEntitler.java

@@ -330,6 +330,6 @@ else if (toolsChannel == null) {
     * @param saltServiceIn The SaltService
     */
    public void setSaltService(SaltService saltServiceIn) {


This seems to be dead code now (as it should be!)

Consider removing.

java/code/src/com/suse/manager/matcher/test/MatcherJsonIOTest.java

java/code/src/com/suse/manager/virtualization/VirtManager.java

java/code/src/com/suse/manager/webui/services/FutureUtils.java

moio · 2020-03-15T17:20:22Z

java/code/src/com/suse/manager/webui/services/impl/SaltService.java

+    }
+
+    /**
+     * {@inheritDoc}


Nitpick: I'd suggest to either have this tag on all public methods or just remove it everywhere.

…necies to handlers

Co-Authored-By: Abid Mehmood <amehmood@suse.de>

moio · 2020-03-20T09:00:28Z

@lucidd is there any plan to downstream this to 4.1?

lucidd · 2020-03-20T09:37:00Z

@moio its already backported to 4.0 https://github.com/SUSE/spacewalk/pull/11009 is there anything else to be done for 4.1?

moio · 2020-03-20T14:01:47Z

@moio its already backported to 4.0 SUSE/spacewalk#11009 is there anything else to be done for 4.1?

Oops, my bad, I wrote 4.1 but meant 4.0. Good that's already backported, I had missed that PR. Thanks.

lucidd requested review from renner, moio, admd and chiaradiamarcelo February 25, 2020 14:54

lucidd force-pushed the salt-interface-uyuni branch 6 times, most recently from 18646cb to 8efbf86 Compare March 2, 2020 16:18

lucidd force-pushed the salt-interface-uyuni branch from 8efbf86 to c8cbafa Compare March 4, 2020 10:20

renner approved these changes Mar 4, 2020

View reviewed changes

lucidd force-pushed the salt-interface-uyuni branch 2 times, most recently from 79dc74a to fb7a408 Compare March 9, 2020 12:52

github-actions bot added the API label Mar 10, 2020

lucidd force-pushed the salt-interface-uyuni branch 2 times, most recently from e2c368a to b1922b2 Compare March 10, 2020 15:00

admd reviewed Mar 11, 2020

View reviewed changes

admd approved these changes Mar 11, 2020

View reviewed changes

lucidd force-pushed the salt-interface-uyuni branch from 7318c81 to 4b1f634 Compare March 12, 2020 12:45

moio reviewed Mar 15, 2020

View reviewed changes

lucidd force-pushed the salt-interface-uyuni branch 2 times, most recently from 558a7b6 to 55681dd Compare March 17, 2020 10:45

lucidd added 7 commits March 19, 2020 17:34

extract salt operations into interface

96ea37e

extract failAfter to utility class

6c32805

refine getAllContainers

9c76743

replace callAsync with specific methods

73c0012

replace callSync with more specific methods

761545b

replace callSync with more specific methods (part1)

a319bba

bootstrap ImageBuildController

82fe6b8

lucidd and others added 23 commits March 19, 2020 17:34

fix compile error

44f85c1

remove reflective handler instanciation to prepare for passing depend…

76dd898

…necies to handlers

bootstrap SaltSSHController

7cefb08

bootstrap NotificationMessageController

77181ce

bootstrap RefreshPillarEventAction

7a88696

fix tests

8a8b2bb

fix checkstyle

388fe50

fix copyright date

30c2708

unify indentation

9cf164a

rename saltService variables to systemQuery

f5be998

move SystemQuery to services.iface package

8fe4478

cleanup unused variable

8a93daa

Fix typo

86f3b9e

Co-Authored-By: Abid Mehmood <amehmood@suse.de>

Fix javadoc

a4d6c78

Co-Authored-By: Abid Mehmood <amehmood@suse.de>

Fix javadoc

5a4f517

Co-Authored-By: Abid Mehmood <amehmood@suse.de>

remove unused unused parameter

de23aab

remove unused constructor

dcf798d

fix ApiHandler

6bbace3

remove unused code

6b818ac

use inheritDoc consistently

d499336

rename handler factory methods and move mocking to test package

d278708

throw descriptive error message when handler does not exist.

be8df23

remove now unused imports

0f8f85a

lucidd force-pushed the salt-interface-uyuni branch from 55681dd to 0f8f85a Compare March 19, 2020 16:35

lucidd merged commit 0f8f85a into master Mar 19, 2020

lucidd deleted the salt-interface-uyuni branch March 19, 2020 17:02

renner mentioned this pull request Mar 20, 2020

modularisation rfc uyuni-project/uyuni-rfc#29

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modularisation Part1 #1954

Modularisation Part1 #1954

lucidd commented Feb 25, 2020 •

edited

Loading

renner left a comment

admd left a comment

moio left a comment

moio Mar 15, 2020

lucidd Mar 17, 2020 •

edited

Loading

moio Mar 15, 2020

lucidd Mar 17, 2020

moio Mar 15, 2020

moio Mar 15, 2020

moio commented Mar 20, 2020

lucidd commented Mar 20, 2020

moio commented Mar 20, 2020

Modularisation Part1 #1954

Modularisation Part1 #1954

Conversation

lucidd commented Feb 25, 2020 • edited Loading

What does this PR change?

GUI diff

Documentation

Test coverage

Links

Changelogs

Re-run a test

renner left a comment

Choose a reason for hiding this comment

admd left a comment

Choose a reason for hiding this comment

moio left a comment

Choose a reason for hiding this comment

moio Mar 15, 2020

Choose a reason for hiding this comment

lucidd Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

moio Mar 15, 2020

Choose a reason for hiding this comment

lucidd Mar 17, 2020

Choose a reason for hiding this comment

moio Mar 15, 2020

Choose a reason for hiding this comment

moio Mar 15, 2020

Choose a reason for hiding this comment

moio commented Mar 20, 2020

lucidd commented Mar 20, 2020

moio commented Mar 20, 2020

lucidd commented Feb 25, 2020 •

edited

Loading

lucidd Mar 17, 2020 •

edited

Loading