MF 1413 - Use per-service URL in SDK #1444

drasko · 2021-08-08T19:45:16Z

Signed-off-by: Drasko DRASKOVIC drasko.draskovic@gmail.com

What does this do?

Fixes SDK to use per-service URL instead of BaseURL.

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

Resolves #1413

Signed-off-by: Drasko DRASKOVIC <drasko.draskovic@gmail.com>

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

Signed-off-by: mteodor <mirko.teodorovic@gmail.com>

codecov-commenter · 2021-08-09T13:01:28Z

Codecov Report

Merging #1444 (37461c8) into master (d6a3830) will increase coverage by 0.03%.
The diff coverage is 50.81%.

@@            Coverage Diff             @@
##           master    #1444      +/-   ##
==========================================
+ Coverage   70.86%   70.90%   +0.03%     
==========================================
  Files         123      123              
  Lines        9600     9564      -36     
==========================================
- Hits         6803     6781      -22     
+ Misses       2270     2256      -14     
  Partials      527      527

Impacted Files	Coverage Δ
certs/service.go	`88.46% <ø> (ø)`
pkg/sdk/go/bootstrap.go	`0.00% <0.00%> (ø)`
pkg/sdk/go/certs.go	`0.00% <0.00%> (ø)`
pkg/sdk/go/groups.go	`0.00% <0.00%> (ø)`
bootstrap/postgres/configs.go	`70.32% <30.00%> (-0.47%)`	⬇️
pkg/sdk/go/users.go	`22.78% <40.00%> (ø)`
pkg/sdk/go/message.go	`34.88% <50.00%> (-0.68%)`	⬇️
pkg/sdk/go/channels.go	`56.52% <100.00%> (-2.16%)`	⬇️
pkg/sdk/go/sdk.go	`100.00% <100.00%> (+3.33%)`	⬆️
pkg/sdk/go/things.go	`57.74% <100.00%> (-1.72%)`	⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6a3830...37461c8. Read the comment docs.

drasko · 2021-08-09T13:05:33Z

bootstrap/api/endpoint_test.go

@@ -160,7 +160,7 @@ func dec(in []byte) ([]byte, error) {
 func newService(auth mainflux.AuthServiceClient, url string) bootstrap.Service {
 	things := mocks.NewConfigsRepository()
 	config := mfsdk.Config{
-		BootstrapURL: url,
+		ThingsURL: url,


Why ThingsURL? It looks that caller is calling this newService() funtion with the URL of Bootstrap server, right? And we are giving this to ThingsURL.

ThingsURL is used to reach things service API endpoints.
Bootstrap test server is created after newService with
bs := newBootstrapServer(svc)
and then you can test bootstrap endpoints using bs.URL

for _, tc := range cases { req := testRequest{ client: bs.Client(), method: http.MethodPost, url: fmt.Sprintf("%s/configs", bs.URL),```

Naming of these functions is confusing. Calling something called newService() inside of Boostrap code, I would expect that it gives me back Boostrap service.

Also, take a look at the callers - what URLs do they provide? URLs of Things? How do you know where Things is - this is something that user should tell you via ENV var, no?

drasko · 2021-08-09T13:06:37Z

certs/service_test.go

@@ -65,7 +65,7 @@ func newService(tokens map[string]string) (certs.Service, error) {

 	auth := thmocks.NewAuthService(tokens)
 	config := mfsdk.Config{
-		CertsURL: server.URL,
+		ThingsURL: server.URL,


What is this server? Is it Bootstrap server? If yes - why we are giving this URL to Things service configuration - in realistic deployment these will be two completely different URLs.

server here is http server serving API calls to things service

Yes, serving HTTP API of a Bootstrap service, i.e. it is a Bootstrap service httptest server. This is why I am saying that this is fundamentally wrong.

not bootstrap, but things

drasko · 2021-08-09T13:06:51Z

bootstrap/service_test.go

@@ -58,6 +58,7 @@ func newService(auth mainflux.AuthServiceClient, url string) bootstrap.Service {
 	things := mocks.NewConfigsRepository()
 	config := mfsdk.Config{
 		BootstrapURL: url,
+		ThingsURL:    url,


Same question here.

answer is similar as above, additionaly providing ThingsURL here is configuring bootstrap service so that it can reach things service API endpoints to create things, channels, check connections

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

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

drasko · 2021-08-09T21:11:06Z

Makefile

@@ -102,12 +102,3 @@ rundev:
 run:
 	docker-compose -f docker/docker-compose.yml up

-runlora:


I removed this clutter because it is rarely launched and should be documented somewhere and not burden the main Makefile.

drasko · 2021-08-09T21:11:46Z

Makefile

-		-f docker/addons/lora-adapter/docker-compose.yml up \
-
-# Run all Mainflux core services except distributed tracing system - Jaeger. Recommended on gateways:
-rungw:


This might be useful, but again - rarely used. Should be rather documented somewhere and executd manually when needed.

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

mteodor · 2021-08-10T10:06:27Z

bootstrap/service_test.go

@@ -57,7 +57,8 @@ var (
 func newService(auth mainflux.AuthServiceClient, url string) bootstrap.Service {
 	things := mocks.NewConfigsRepository()
 	config := mfsdk.Config{
-		BaseURL: url,
+		BootstrapURL: url,


we dont need this BootstrapURL

mteodor · 2021-08-10T10:14:20Z

pkg/sdk/go/things_test.go

-		TLSVerification:   false,
+		ThingsURL:       ts.URL,
+		MsgContentType:  contentType,
+		TLSVerification: false,


can we perform tests with TLSVerification = true

dborovcanin · 2021-08-10T10:26:37Z

provision/configs/config.toml

  tls = true
-  users_location = ""
+  users_url = ""


Why not defaulting to the correct Users' service URL? Also, TLS is on, but URLs are http.

Update tests, remove linter warnings, remove dead code. Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

* Use per-service URL in SDK Signed-off-by: Drasko DRASKOVIC <drasko.draskovic@gmail.com> * Fix CLI Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Fix CLI messaging Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Fix message tests Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Simplify Bootstrap Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Update API doc and responses Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * fix failing certs, bootstrap tests Signed-off-by: mteodor <mirko.teodorovic@gmail.com> * fix failing certs, bootstrap tests Signed-off-by: mteodor <mirko.teodorovic@gmail.com> * Fix tests and rename to auth service Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Clean the code Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Remove unnecessary Repository logs Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Always return error in case of repo failure Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Cleanup SDK and CLI Update tests, remove linter warnings, remove dead code. Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Clean the code Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Undo Bootstrap changes Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Fix tests Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Fix linter Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> Co-authored-by: mteodor <mirko.teodorovic@gmail.com> Co-authored-by: dusanb94 <dusan.borovcanin@mainflux.com> Signed-off-by: Burak Sekili <buraksekili@gmail.com>

* Use per-service URL in SDK Signed-off-by: Drasko DRASKOVIC <drasko.draskovic@gmail.com> * Fix CLI Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Fix CLI messaging Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Fix message tests Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Simplify Bootstrap Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Update API doc and responses Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * fix failing certs, bootstrap tests Signed-off-by: mteodor <mirko.teodorovic@gmail.com> * fix failing certs, bootstrap tests Signed-off-by: mteodor <mirko.teodorovic@gmail.com> * Fix tests and rename to auth service Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Clean the code Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Remove unnecessary Repository logs Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Always return error in case of repo failure Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Cleanup SDK and CLI Update tests, remove linter warnings, remove dead code. Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com> * Clean the code Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Undo Bootstrap changes Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Fix tests Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> * Fix linter Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com> Co-authored-by: mteodor <mirko.teodorovic@gmail.com> Co-authored-by: dusanb94 <dusan.borovcanin@mainflux.com>

drasko added 6 commits July 31, 2021 17:37

Use per-service URL in SDK

f465c37

Signed-off-by: Drasko DRASKOVIC <drasko.draskovic@gmail.com>

Fix CLI

8bc6575

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

Fix CLI messaging

55a3c8a

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

Fix message tests

6042fb0

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

Simplify Bootstrap

cf9404c

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

Update API doc and responses

65ba37d

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

drasko requested a review from a team as a code owner August 8, 2021 19:45

mteodor added 2 commits August 9, 2021 14:17

fix failing certs, bootstrap tests

71ba7b5

Signed-off-by: mteodor <mirko.teodorovic@gmail.com>

fix failing certs, bootstrap tests

7d9e421

Signed-off-by: mteodor <mirko.teodorovic@gmail.com>

drasko commented Aug 9, 2021

View reviewed changes

drasko and others added 3 commits August 9, 2021 16:24

Fix tests and rename to auth service

d4d2cc4

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

Clean the code

979fdee

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

Remove unnecessary Repository logs

664f822

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

drasko commented Aug 9, 2021

View reviewed changes

Always return error in case of repo failure

21e4c51

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

mteodor reviewed Aug 10, 2021

View reviewed changes

dborovcanin reviewed Aug 10, 2021

View reviewed changes

dborovcanin and others added 5 commits August 10, 2021 13:35

Cleanup SDK and CLI

34e5bc4

Update tests, remove linter warnings, remove dead code. Signed-off-by: dusanb94 <dusan.borovcanin@mainflux.com>

Clean the code

da5d9ab

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

Undo Bootstrap changes

9df5703

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

Fix tests

2cb9e23

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

Fix linter

37461c8

Signed-off-by: Drasko Draskovic <drasko.draskovic@gmail.com>

dborovcanin approved these changes Aug 11, 2021

View reviewed changes

drasko merged commit d73a5d5 into master Aug 11, 2021

manuio deleted the sdk branch May 14, 2022 10:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF 1413 - Use per-service URL in SDK #1444

MF 1413 - Use per-service URL in SDK #1444

drasko commented Aug 8, 2021

codecov-commenter commented Aug 9, 2021 •

edited

Loading

drasko Aug 9, 2021

mteodor Aug 9, 2021

drasko Aug 9, 2021

drasko Aug 9, 2021

mteodor Aug 9, 2021

drasko Aug 9, 2021

mteodor Aug 10, 2021

drasko Aug 9, 2021

mteodor Aug 9, 2021

drasko Aug 9, 2021

drasko Aug 9, 2021

mteodor Aug 10, 2021

mteodor Aug 10, 2021

dborovcanin Aug 10, 2021

MF 1413 - Use per-service URL in SDK #1444

MF 1413 - Use per-service URL in SDK #1444

Conversation

drasko commented Aug 8, 2021

What does this do?

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

codecov-commenter commented Aug 9, 2021 • 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

Choose a reason for hiding this comment

codecov-commenter commented Aug 9, 2021 •

edited

Loading