new storage #717

gyorb · 2017-07-10T11:14:42Z

Resolves #683

Xazax-hun

Some initial comments, not a throughout review, since it is still work in progress.

Xazax-hun · 2017-07-10T11:18:09Z

api/report_viewer_server.thrift

+    // store checker run related data to the database
+    // by default updates the results if name was already found
+    // using the force flag removes existing analysis results for a run
+    i64  addCheckerRun(


The comments should be full sentences.

Does the force parameter make sense in the new model? Maybe it should be removed?

Xazax-hun · 2017-07-10T11:19:20Z

api/report_viewer_server.thrift

+                       4: bool force)
+                       throws (1: shared.RequestFailed requestError),
+
+    bool replaceConfigInfo(


Since the whole run is stored again in incremental mode, maybe this API call no longer makes sense.

Xazax-hun · 2017-07-10T11:23:21Z

libcodechecker/analyze/analyzers/analyzer_types.py

@@ -22,6 +21,7 @@
 from libcodechecker.analyze.analyzers import result_handler_clang_tidy
 from libcodechecker.analyze.analyzers import result_handler_plist_to_db
 from libcodechecker.analyze.analyzers import result_handler_plist_to_stdout
+from libcodechecker.cmd.cmd_line_client import setup_client


It is a pretty strange dependency for this module.

setup_client handles authentication and creates the local Thrift endpoint that can be used (after an optional authentication, if needed) to transmit data.

I would definitely introduce, akin to the other imports like this, a notice:

# TODO: This is a cross-subpackage reference.

Or maybe such code should be pulled out from the cmd_line_client into a separate helper module to avoid layering violations.

That's why I'd go with introducing a TODO at this point. There are a lot of such TODOs that are to be handled later on. I would like this pull request to be minimal in terms of changes related to the new storage system, for review and history's sake.

Cross-package imports are another concern, they can be refactored out in a batch.

In these scenarios I am more fond of having a separate pull request to pull this code out and rebase this one after that one is accepted, to avoid the excess amount of TODOs.

Yes I know this is not the way it should be handled but I did not wanted to do more refactoring during the development and importing this way was easy to get the functionality.

I've factored out to an api_client package which can be used by the command line client and the store.

Xazax-hun · 2017-07-10T11:25:37Z

libcodechecker/analyze/analyzers/result_handler_plist_to_db.py

+                                  self.buildaction.analyzer_type,
+                                  source_file_name)
+
+        assert self.analyzer_returncode == 0


Maybe assert is not the best way to handle the error here. The return code is not zero is not due to a programming error. It might be a configuration error.

Actually after splitting up the analyze and store functionality the analyzer return code is set in store.py which is not connected to the actual analysis.
Checking this value with the new setup is not necessary.

whisperity · 2017-07-10T11:18:02Z

README.md

-
-### Start a web server
-
+Start a CodeChecker web and storage server in another terminal or as a background process. By default it will listen on `localhost:8001'


Sentence is not terminated.

whisperity · 2017-07-10T11:26:55Z

api/report_viewer_server.thrift

+                       1: string command,
+                       2: string name,
+                       3: string version,
+                       4: bool force)


In general, please normalise the style of this file. At some places, the closing ) of the argument list is newline, in some cases (like this one), it is not.

whisperity · 2017-07-10T11:27:48Z

api/report_viewer_server.thrift

@@ -279,6 +285,93 @@ service codeCheckerDBAccess {
                                             2: i64 new_run_id,
                                             3: DiffType diff_type,
                                             4: ReportFilterList reportFilters)
-                                             throws (1: shared.RequestFailed requestError)
+                                             throws (1: shared.RequestFailed requestError),


Does calling this file report_viewer_server.thrift proper anymore? It integrates multiple functionality. Maybe report_server.thrift?

☑️ This has been done, just GitHub is not picking it up. 😉

whisperity · 2017-07-10T11:28:28Z

api/report_viewer_server.thrift

+
+
+
+


I'd rather, though, introduce some comments that separate the functionality handled by this service.

whisperity · 2017-07-10T11:29:17Z

docs/user_guide.md

@@ -86,6 +87,12 @@ _CodeChecker_. It analyzes your project and stores the reported code defects
 in a database, which can be viewed in a Web Browser later on (via `CodeChecker
 server`).

+With the server command start the server where the results will be stored and


I would somehow merge this with the previous paragraph. CodeChecker server is mentioned twice.

whisperity · 2017-07-10T11:30:43Z

libcodechecker/libhandlers/store.py

@@ -100,6 +118,20 @@ def add_arguments_to_parser(parser):
                             "the '--name' parameter given to 'codechecker-"
                             "analyze' will be used, if exists.")

+    parser.add_argument('--host',


We should introduce an argument_group for these arguments and explain that they are used to specify where the resultes are stored.

whisperity · 2017-07-10T11:31:05Z

libcodechecker/libhandlers/check.py

@@ -152,6 +152,20 @@ def add_arguments_to_parser(parser):
                             "the '--name' parameter given to 'codechecker-"
                             "analyze' will be used, if exists.")

+    parser.add_argument('--host',


(Same as comment in store.py.)

whisperity · 2017-07-10T11:37:27Z

libcodechecker/libhandlers/store.py

+                        default='localhost',
+                        help="CodeChecker server host.")
+
+    parser.add_argument('--port',


CodeChecker server and CodeChecker cmd has -p as shorthand for --port.

whisperity

Very minor stuff. Otherwise, the code looks good.

I'll come up with better help messages, though.

whisperity · 2017-07-11T08:27:28Z

libcodechecker/cmd/cmd_line_client.py

-        LOG.critical("No authentication methods were reported by the server "
-                     "that this client could support.")
-        sys.exit(1)
+        login = False


Statement can be simplified: login = 'logout' in args. And then the variable can be inlined into the function call.

whisperity · 2017-07-11T08:29:35Z

libcodechecker/cmd/cmd_line_client.py

 from libcodechecker import suppress_file_handler
+from libcodechecker.api_client.client import setup_client


Minor thing, but I don't like the package name for this. We should make it somehow inline with the not-subcommand-related libcodechecker subfolders, like libauth or libhandlers.

Maybe it could go straight into libauth? Or libclient? Something with lib on the beginning of it.

I would choose the libclient name.

whisperity · 2017-07-11T08:31:04Z

tests/functional/suppress/test_suppress.py

@@ -11,6 +11,7 @@
 import unittest

 from codeCheckerDBAccess.ttypes import ReportFilter
+from shared.ttypes import SuppressBugData


Is this import used?

whisperity · 2017-07-11T08:31:24Z

tests/functional/report_viewer_api/test_hash_clash.py

@@ -12,6 +12,7 @@
 import random
 import string
 import unittest
+import base64


Import name alphabetical order violated.

whisperity · 2017-07-11T12:30:57Z

@gyorb There is an issue. CodeChecker cmd login on an authentication-requiring server simply states that you need to authenticate (via CodeChecker cmd login) and then the process exits. (With return code 0.)

gyorb · 2017-07-11T14:07:37Z

@whisperity the login problem should be fixed now

whisperity · 2017-07-12T07:14:04Z

CodeChecker cmd login --logout is broken. It states "You are already logged in." and does not terminate the session.

whisperity · 2017-07-12T07:16:09Z

libcodechecker/libclient/client.py

+
+        if auth_token and handshake.sessionStillActive:
+            LOG.info("You are already logged in.")
+            return


This early return prevents logout.

Fixed this.

whisperity

Apart from the --logout issue mentioned earlier (that was not made broken in the current changeset, it seems it was broken and not triaged earlier!) and a little comment, looking good. 😉

whisperity · 2017-07-12T07:20:10Z

libcodechecker/analyze/analyzers/analyzer_types.py

-            run_id)
-
-    elif buildaction.analyzer_type == CLANG_TIDY:
-        res_handler = result_handler_clang_tidy.ClangTidyPlistToDB(


Removing this distintiction here made result_handler_clang_tidy.ClangTidyPlistToDB an obsolete class. Is this intended?

Yes the storing of plist files to the database should not depend on the analyzer which generated it.
This functionality should be refactored so that there should be only one plist to database handler.

I did not wanted to increase the size of this pull request with this refactoring. It should be a separate development.

whisperity

Oh, yeah, one more thing: We should introduce a simple test case somewhere that covers (maybe tests/functional/authentication is the best place for this?) getting an error if store is called with a server and without authentication.

One server should handle all of the requests to store and view the results. * merge report server api and implementation into the report viewer * only one common server is started to store and view the results * authentication is introduced for the storage api too * store and check should use a codechecker server host and port no direct connection to the database is available * database arguments in store and check are deprecated * authentication and client api was factored out from the command line client modul to a common client module * new test which tries to store results to an authenticated server

gyorb added the WIP 💣 Work In Progress label Jul 10, 2017

Xazax-hun reviewed Jul 10, 2017

View reviewed changes

whisperity requested changes Jul 10, 2017

View reviewed changes

gyorb force-pushed the one_server branch 5 times, most recently from 8127d19 to 395d679 Compare July 10, 2017 16:05

whisperity requested changes Jul 11, 2017

View reviewed changes

gyorb force-pushed the one_server branch 5 times, most recently from 189814a to 5307da2 Compare July 11, 2017 11:54

gyorb force-pushed the one_server branch from 5307da2 to 62fc52d Compare July 11, 2017 14:07

gyorb force-pushed the one_server branch 2 times, most recently from 55f1a18 to f1aadbf Compare July 11, 2017 15:54

whisperity reviewed Jul 12, 2017

View reviewed changes

whisperity requested changes Jul 12, 2017

View reviewed changes

gyorb force-pushed the one_server branch from f1aadbf to 8c9b87f Compare July 12, 2017 12:16

gyorb force-pushed the one_server branch from 8c9b87f to 9aa995d Compare July 12, 2017 12:30

gyorb removed the WIP 💣 Work In Progress label Jul 12, 2017

whisperity approved these changes Jul 12, 2017

View reviewed changes

dkrupp merged commit 598ead7 into Ericsson:master Jul 12, 2017

This was referenced Jul 14, 2017

Update documentation #730

Merged

Make server port mandatory in command line clients #740

Closed

Fix server ignoring direct --sqlite db specification #743

Merged

gyorb deleted the one_server branch October 16, 2017 06:26


		### Start a web server

		Start a CodeChecker web and storage server in another terminal or as a background process. By default it will listen on `localhost:8001'

		from libcodechecker import suppress_file_handler
		from libcodechecker.api_client.client import setup_client

new storage #717

new storage #717

Conversation

gyorb commented Jul 10, 2017

Xazax-hun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity commented Jul 11, 2017

gyorb commented Jul 11, 2017

whisperity commented Jul 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whisperity left a comment

Choose a reason for hiding this comment

whisperity Jul 10, 2017 •

edited

Loading

whisperity left a comment •

edited

Loading