MF-1556 - Move the most used functions in main.go to internal package #1601

arvindh123 · 2022-05-03T18:25:17Z

What does this do?

This pull request for moving the HTTP Server and GRPC server from cmd/service/main.go to internal/mfserver package

Which issue(s) does this PR fix/relate to?

Resolve partially #1556 because need to check other functions feasibility to move to internal package

List any changes that modify/break current functionality

N/A

Have you included tests for your changes?

N/A

Did you document any new/modified functionality?

N/A

Notes

N/A

codecov-commenter · 2022-05-03T18:38:43Z

Codecov Report

Merging #1601 (72c26ca) into master (ada5813) will increase coverage by 0.06%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #1601      +/-   ##
==========================================
+ Coverage   70.32%   70.38%   +0.06%     
==========================================
  Files         148      146       -2     
  Lines       11501    11394     -107     
==========================================
- Hits         8088     8020      -68     
+ Misses       2745     2724      -21     
+ Partials      668      650      -18

Impacted Files	Coverage Δ
certs/postgres/certs.go	`0.00% <ø> (ø)`
readers/postgres/init.go	`89.47% <ø> (ø)`
readers/postgres/messages.go	`85.29% <ø> (ø)`
readers/timescale/init.go	`89.47% <ø> (ø)`
auth/postgres/init.go	`100.00% <100.00%> (+5.00%)`	⬆️
bootstrap/postgres/init.go	`100.00% <100.00%> (+5.97%)`	⬆️
certs/postgres/init.go	`100.00% <100.00%> (+22.58%)`	⬆️
certs/service.go	`83.13% <100.00%> (-0.21%)`	⬇️
consumers/notifiers/postgres/init.go	`100.00% <100.00%> (+14.28%)`	⬆️
consumers/writers/postgres/init.go	`100.00% <100.00%> (+10.52%)`	⬆️
... and 3 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

cmd/auth/main.go

internal/apiutil/mfserver/grpcserver/grpcserver.go

cmd/auth/main.go

internal/apiutil/mfserver/mfserver.go

cmd/auth/main.go

dborovcanin · 2022-05-23T10:45:35Z

internal/apiutil/mfdatabase/connect.go

@@ -0,0 +1,41 @@
+// Copyright (c) Mainflux


API util does not look like a good place for this. Something like internal/db/connect.go. Also, the most commonly used DB connection (Postgres) is missing.

So with Postgres, there is a slight issue. For instance when the auth service connects to Postgres, it will do a migration which is specific to the auth service and hence the bootstrap can't use it.
If we implement a common connect to Postgres this would mean we will import the different modules. Is that okay?

@dborovcanin, Moving of postgres connect to internal package could be done in this PR or could we do it in seperate PR?

dborovcanin · 2022-05-23T10:51:44Z

internal/apiutil/metrics.go

+	stdprometheus "github.com/prometheus/client_golang/prometheus"
+)
+
+func MakeMetrics(svcName string) (*kitprometheus.Counter, *kitprometheus.Summary) {


No need for this complex logic. You can simply pass namespace and subsystem and use:

counter := kitprometheus.NewCounterFrom(stdprometheus.CounterOpts{ Namespace: namespace, Subsystem: subsystem, Name: "request_count", Help: "Number of requests received.", }, []string{"method"}) latency := kitprometheus.NewSummaryFrom(stdprometheus.SummaryOpts{ Namespace: namespace, Subsystem: subsystem, Name: "request_latency_microseconds", Help: "Total duration of requests in microseconds.", }, []string{"method"})

Do we need to differentiate the help message between a reader and a writer?

@dborovcanin, changed, Please reveiew this part and give your comments,

dborovcanin · 2022-05-23T10:53:54Z

internal/apiutil/auth.go

@@ -0,0 +1,39 @@
+// Copyright (c) Mainflux


Consider renaming apiutil to something closer to what this package is for - maybe initutil or just init.

@dborovcanin, The init is misleading so we decided to use apiutil. And the internal folder is restrucutred.
Please review the latest code and give your comments

dborovcanin · 2022-05-23T10:56:36Z

internal/apiutil/mfserver/grpcserver/grpcserver.go

+type GRPCServer struct {
+	mfserver.BaseServer
+	server     *grpc.Server
+	authSrvSvr mainflux.AuthServiceServer


Auth service sounds implementation-specific. What with other gRPC services, such as ThingsServiceServer?

dborovcanin · 2022-05-23T10:58:30Z

internal/apiutil/mfserver/mfserver.go

+}
+
+func stopAllServer(servers ...Server) error {
+	var err error


What's this error for?

To be able to aggregate all the errors. For instance, we have two errors out of 5 servers, we will be able to return all the errors rather than the error it checked first

auth/api/grpc/requests.go

manuio · 2022-05-24T11:47:37Z

auth/api/grpc/server.go

@@ -11,7 +11,7 @@ import (
 	"github.com/golang/protobuf/ptypes/empty"
 	mainflux "github.com/mainflux/mainflux"
 	"github.com/mainflux/mainflux/auth"
-	"github.com/mainflux/mainflux/internal/apiutil"
+	apiutil "github.com/mainflux/mainflux/internal/init"


same here and for other imports

Renaming to initutil

auth/api/grpc/requests.go

manuio · 2022-05-24T14:29:55Z

cmd/http/main.go

 	"github.com/mainflux/mainflux"
 	adapter "github.com/mainflux/mainflux/http"
 	"github.com/mainflux/mainflux/http/api"
+	initutil "github.com/mainflux/mainflux/internal/init"
+	"github.com/mainflux/mainflux/internal/init/mfserver"
+	"github.com/mainflux/mainflux/internal/init/mfserver/httpserver"


This is quite redundant. I'd use "github.com/mainflux/mainflux/internal/servers/http"
I'd prpose the next organisation:

internal/auth/auth.go internal/auth/keto.go internal/auth/things.go internal/db/redis.go internal/db/mongo.go internal/server/http.go internal/server/coap.go internal/server/grpc.go internal/apiutil/transport.go internal/apiutil/responses.go internal/apiutil/tokens.go internal/apiutil/errors.go

@manuio, changed as per you proposal

manuio · 2022-05-24T15:45:36Z

cmd/auth/main.go

@@ -19,8 +19,8 @@ import (
 	"github.com/mainflux/mainflux/auth/tracing"
 	"github.com/mainflux/mainflux/internal"
 	"github.com/mainflux/mainflux/internal/server"
-	"github.com/mainflux/mainflux/internal/server/grpcserver"
-	"github.com/mainflux/mainflux/internal/server/httpserver"
+	mfgrpcserver "github.com/mainflux/mainflux/internal/server/grpc"


I'd remove mf prefix

@manuio, removed mf prefix

manuio · 2022-05-25T08:00:38Z

auth/api/grpc/server.go

@@ -253,6 +253,6 @@ func encodeError(err error) error {
 	case errors.Contains(err, errors.ErrAuthorization):
 		return status.Error(codes.PermissionDenied, err.Error())
 	default:
-		return status.Error(codes.Internal, "internal server error")
+		return status.Error(codes.Internal, "apiutil server error")


What this means? Can we find something more meaningfull?

@manuio ,changed to internal server error

cmd/bootstrap/main.go

internal/starter/loadConfig.go

dborovcanin

If the lib @drasko suggested works for us - I'd go for it. It looks well written, tested, broadly used, and lightweight enough for us. I like having our solution, but we'd need to invest in testing and maintenance with no obvious benefits.

dborovcanin · 2022-09-20T16:19:08Z

internal/server/server.go

+	Start() error
+	Stop() error


Maybe Serve and Shutdown are more similar to Go HTTP server terminology.

dborovcanin · 2022-09-20T16:30:20Z

internal/server/server.go

+	Ctx      context.Context
+	Cancel   context.CancelFunc
+	Name     string
+	Address  string


Do we need Address field?

@dborovcanin, Yes required. Now it is used properly.

internal/server/http/http.go

internal/server/grpc/grpc.go

* Add : errgroup to cmd/auth Signed-off-by: Arvindh <arvindh91@gmail.com> * Add : Handle graceful stop for auth service Remove : errgroups from auth service Signed-off-by: Arvindh <arvindh91@gmail.com> * Add : Wait till server shutdown Signed-off-by: Arvindh <arvindh91@gmail.com> * Change : instead of waitgroup changed to errgroups Signed-off-by: Arvindh <arvindh91@gmail.com> * change : KillSignalHandler return type to error Signed-off-by: Arvindh <arvindh91@gmail.com> * Empty Commit Signed-off-by: Arvindh <arvindh91@gmail.com> * Add : Context to http server shutdown Rename : varaible from proto to protocol Signed-off-by: Arvindh <arvindh91@gmail.com> * change : to default log level Signed-off-by: Arvindh <arvindh91@gmail.com> * Add : Sign-off Signed-off-by: Arvindh <arvindh91@gmail.com> * Add: graceful stop of http and grpc server Signed-off-by: Arvindh <arvindh91@gmail.com> * Fix: typos and caps Signed-off-by: Arvindh <arvindh91@gmail.com> * Add: Signed-off Signed-off-by: Arvindh <arvindh91@gmail.com> * Rename: Func KillSignalHandler to SignalHandler Add: SIGABRT Signed-off-by: Arvindh <arvindh91@gmail.com> * Fix: auth service Signed-off-by: Arvindh <arvindh91@gmail.com> * Add: timeout for grpc gracefulstop Fix: typos Signed-off-by: Arvindh <arvindh91@gmail.com> * Add: .vscode folder to git ignore Signed-off-by: Arvindh <arvindh91@gmail.com> * change: variable name to stopWaitTime Signed-off-by: Arvindh <arvindh91@gmail.com> * remove: .vscode folder Signed-off-by: Arvindh <arvindh91@gmail.com> * remove: .vscode from .gitignore Signed-off-by: Arvindh <arvindh91@gmail.com> * Add : logger to handlers Signed-off-by: Arvindh <arvindh91@gmail.com> * Add : New line at end of .gitignore file Signed-off-by: Arvindh <arvindh91@gmail.com> * Fix : variable naming Add : graceful stop for timescale Signed-off-by: Arvindh <arvindh91@gmail.com> * Remove : unsued NATS library from import Signed-off-by: Arvindh <arvindh91@gmail.com> * Move: "https" and "https" to moved to const var Signed-off-by: Arvindh <arvindh91@gmail.com> * Move: "http" and "https" to moved to const var Signed-off-by: Arvindh <arvindh91@gmail.com> * update: branch with master Signed-off-by: Arvindh <arvindh91@gmail.com> Co-authored-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> Co-authored-by: Drasko DRASKOVIC <drasko.draskovic@gmail.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

Signed-off-by: Arvindh <arvindh91@gmail.com>

* Initial commit Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Update subscriber interface Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Add tests Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Add tests Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * check subscription map Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Check topic id after topic Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * reword description Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Setup empty queue Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Change mqtt implementation Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Switch statements Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Simplify Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Change mqtt subscriber Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Protect subscription map Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Fix subscription Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Set client id Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Format Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Change delete method Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> Co-authored-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

* Add max and min limit size Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Format Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> * Format Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> Co-authored-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

Signed-off-by: zhangchuanfeng <654300242@qq.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

Signed-off-by: Arvindh <arvindh91@gmail.com>

arvindh123 · 2023-01-25T11:20:41Z

Rename internal/client to internal/clients.

Done

auth/postgres/init.go

certs/postgres/init.go

things/postgres/init.go

Signed-off-by: Arvindh <arvindh91@gmail.com>

Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>

drasko

This is all good, just please remove full-stops from one-sentence comments and I will approve.

drasko · 2023-02-01T23:27:11Z

auth/postgres/init.go

-
-func migrateDB(db *sqlx.DB) error {
-	migrations := &migrate.MemoryMigrationSource{
+// Migration of Auth service.


No need for a full-stop at the end of the comment. Only if the comment spans more than one sentences, then putting full-stops makes sense.

Removed end dot in single line comments

Signed-off-by: Arvindh <arvindh91@gmail.com>

… MF-1556

Signed-off-by: Arvindh <arvindh91@gmail.com>

drasko

LGTM

Signed-off-by: Arvindh <arvindh91@gmail.com>

drasko

LGTM

dborovcanin changed the title ~~MF-1556 - Moving the HTTP Server and GRPC server to internal~~ MF-1556 - Move the most used functions in main.go to internal package May 3, 2022

mteodor reviewed May 10, 2022

View reviewed changes

cmd/auth/main.go Outdated Show resolved Hide resolved

mteodor reviewed May 10, 2022

View reviewed changes

internal/apiutil/mfserver/grpcserver/grpcserver.go Outdated Show resolved Hide resolved

mteodor reviewed May 10, 2022

View reviewed changes

cmd/auth/main.go Outdated Show resolved Hide resolved

mteodor reviewed May 10, 2022

View reviewed changes

internal/apiutil/mfserver/mfserver.go Outdated Show resolved Hide resolved

manuio reviewed May 23, 2022

View reviewed changes

cmd/auth/main.go Outdated Show resolved Hide resolved

dborovcanin requested changes May 23, 2022

View reviewed changes

dborovcanin marked this pull request as ready for review May 24, 2022 07:38

dborovcanin requested a review from a team as a code owner May 24, 2022 07:38

manuio suggested changes May 24, 2022

View reviewed changes

manuio reviewed May 24, 2022

View reviewed changes

auth/api/grpc/requests.go Outdated Show resolved Hide resolved

manuio reviewed May 24, 2022

View reviewed changes

manuio suggested changes May 25, 2022

View reviewed changes

manuio suggested changes Jun 14, 2022

View reviewed changes

cmd/bootstrap/main.go Outdated Show resolved Hide resolved

drasko requested changes Aug 29, 2022

View reviewed changes

internal/starter/loadConfig.go Outdated Show resolved Hide resolved

internal/starter/loadConfig.go Outdated Show resolved Hide resolved

dborovcanin requested changes Sep 20, 2022

View reviewed changes

arvindh123 and others added 12 commits December 9, 2022 20:34

Add: httpserver and grpcsever

eecacbf

Signed-off-by: Arvindh <arvindh91@gmail.com>

move http and grpc server functions

cd33175

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

Move Keto and Jaeger

38cf38b

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

Add metrics and auth

94a7d60

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

NOISSUE - Retrieve client key on cert issuing (absmach#1607)

d8e6ca7

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

fix bug (absmach#1604)

c96ddd3

Signed-off-by: zhangchuanfeng <654300242@qq.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

Rename service name

495ad42

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

Change metrics method

80bccdd

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

Rename

0f78937

Signed-off-by: 0x6f736f646f <blackd0t@protonmail.com> Signed-off-by: Arvindh <arvindh91@gmail.com>

arvindh123 added 2 commits January 25, 2023 16:44

rename: directory

b4b5efc

Signed-off-by: Arvindh <arvindh91@gmail.com>

rename: variable

6804998

Signed-off-by: Arvindh <arvindh91@gmail.com>

dborovcanin requested changes Jan 25, 2023

View reviewed changes

auth/postgres/init.go Outdated Show resolved Hide resolved

certs/postgres/init.go Outdated Show resolved Hide resolved

things/postgres/init.go Outdated Show resolved Hide resolved

arvindh123 and others added 2 commits January 25, 2023 22:18

remove: unused librar

f5596bf

Signed-off-by: Arvindh <arvindh91@gmail.com>

Format code and remove unused comments

95c95af

Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>

dborovcanin previously approved these changes Jan 25, 2023

View reviewed changes

Fix tests

a4a57b5

Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>

dborovcanin dismissed their stale review via a4a57b5 January 26, 2023 07:47

dborovcanin previously approved these changes Jan 26, 2023

View reviewed changes

Move test URL construction out of the loop

7227657

Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>

dborovcanin dismissed their stale review via 7227657 January 26, 2023 10:21

dborovcanin previously approved these changes Jan 26, 2023

View reviewed changes

drasko requested changes Feb 1, 2023

View reviewed changes

arvindh123 added 3 commits February 2, 2023 11:42

rebase with master

4b2d804

Signed-off-by: Arvindh <arvindh91@gmail.com>

Merge branch 'MF-1556' of https://github.com/arvindh123/mainflux into…

371e167

… MF-1556

remove end dot in single line comments

b9a5a60

Signed-off-by: Arvindh <arvindh91@gmail.com>

arvindh123 dismissed dborovcanin’s stale review via b9a5a60 February 2, 2023 06:41

empty

cc16a86

Signed-off-by: Arvindh <arvindh91@gmail.com>

drasko previously approved these changes Feb 2, 2023

View reviewed changes

Merge branch 'master' into MF-1556

72c26ca

Signed-off-by: Arvindh <arvindh91@gmail.com>

arvindh123 dismissed drasko’s stale review via 72c26ca February 3, 2023 13:44

arvindh123 requested review from drasko February 3, 2023 14:23

dborovcanin approved these changes Feb 3, 2023

View reviewed changes

drasko approved these changes Feb 3, 2023

View reviewed changes

drasko merged commit d00d13d into absmach:master Feb 3, 2023

dborovcanin deleted the MF-1556 branch February 4, 2023 12:33

rodneyosodo mentioned this pull request Jul 24, 2023

NOISSUE - Move HTTP and GRPC Server Implementations To Internal ultravioletrs/agent-OLD#25

Merged

arvindh123 mentioned this pull request May 15, 2024

Consider using Envconfig #1286

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-1556 - Move the most used functions in main.go to internal package #1601

MF-1556 - Move the most used functions in main.go to internal package #1601

arvindh123 commented May 3, 2022 •

edited

Loading

codecov-commenter commented May 3, 2022 •

edited

Loading

dborovcanin May 23, 2022

rodneyosodo May 23, 2022

arvindh123 Dec 13, 2022

dborovcanin May 23, 2022

rodneyosodo May 23, 2022

arvindh123 Dec 13, 2022

dborovcanin May 23, 2022

arvindh123 Dec 13, 2022

dborovcanin May 23, 2022

dborovcanin May 23, 2022

rodneyosodo May 24, 2022

manuio May 24, 2022

rodneyosodo May 24, 2022

manuio May 24, 2022 •

edited

Loading

arvindh123 Dec 13, 2022

manuio May 24, 2022

arvindh123 Dec 13, 2022

manuio May 25, 2022 •

edited

Loading

arvindh123 Dec 13, 2022

dborovcanin left a comment

dborovcanin Sep 20, 2022

dborovcanin Sep 20, 2022

arvindh123 Dec 13, 2022

arvindh123 commented Jan 25, 2023

drasko left a comment

drasko Feb 1, 2023

arvindh123 Feb 2, 2023

drasko left a comment

drasko left a comment

MF-1556 - Move the most used functions in main.go to internal package #1601

MF-1556 - Move the most used functions in main.go to internal package #1601

Conversation

arvindh123 commented May 3, 2022 • edited Loading

What does this do?

Which issue(s) does this PR fix/relate to?

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

Notes

codecov-commenter commented May 3, 2022 • edited Loading

Codecov Report

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

manuio May 24, 2022 • 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

manuio May 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dborovcanin 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

arvindh123 commented Jan 25, 2023

drasko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

arvindh123 commented May 3, 2022 •

edited

Loading

codecov-commenter commented May 3, 2022 •

edited

Loading

manuio May 24, 2022 •

edited

Loading

manuio May 25, 2022 •

edited

Loading