MF-1699 - Enrich SDK and CLI #1719

rodneyosodo · 2023-02-07T09:40:25Z

What does this do?

Enriches SDK and CLI. Also it updated the api docs swagger files.

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

Resolves #1699
Duplicate of #1711

List any changes that modify/break current functionality

N/A

Have you included tests for your changes?

Yes

Did you document any new/modified functionality?

No

Notes

N/A

codecov-commenter · 2023-02-07T10:07:04Z

Codecov Report

Merging #1719 (797dc4d) into master (12a3214) will decrease coverage by 0.48%.
The diff coverage is 25.82%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master    #1719      +/-   ##
==========================================
- Coverage   70.29%   69.82%   -0.48%     
==========================================
  Files         146      149       +3     
  Lines       11518    11697     +179     
==========================================
+ Hits         8097     8167      +70     
- Misses       2765     2865     +100     
- Partials      656      665       +9

Impacted Files	Coverage Δ
certs/service.go	`83.13% <0.00%> (ø)`
pkg/sdk/go/bootstrap.go	`0.00% <0.00%> (ø)`
pkg/sdk/go/policies.go	`0.00% <0.00%> (ø)`
pkg/sdk/go/responses.go	`0.00% <ø> (ø)`
pkg/sdk/go/sdk.go	`72.30% <ø> (ø)`
pkg/sdk/go/things.go	`41.55% <0.00%> (-31.17%)`	⬇️
pkg/sdk/go/consumers.go	`48.64% <48.64%> (ø)`
pkg/sdk/go/certs.go	`76.19% <77.77%> (+76.19%)`	⬆️
certs/certs.go	`83.33% <83.33%> (ø)`
auth/api/http/groups/transport.go	`45.79% <100.00%> (ø)`

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

dborovcanin

Please update the PR.

drasko · 2023-03-12T15:41:38Z

pkg/sdk/go/things.go

@@ -169,9 +237,65 @@ func (sdk mfSDK) Connect(connIDs ConnectionIDs, token string) errors.SDKError {
 	return sdkerr
 }

+func (sdk mfSDK) DisConnect(connIDs ConnectionIDs, token string) errors.SDKError {


Why not Disconnect?

SammyOina · 2023-03-21T20:32:48Z

certs/certs.go

@@ -30,3 +39,46 @@ type Repository interface {
 	// RetrieveBySerial retrieves a certificate for a given serial ID
 	RetrieveBySerial(ctx context.Context, ownerID, serialID string) (Cert, error)
 }
+
+func LoadCertificates(caPath, caKeyPath string) (tls.Certificate, *x509.Certificate, error) {
+	var tlsCert tls.Certificate


unnecessary initialization of tlsCert and caCert considering they're assigned in LoadX509KeyPair and ReadCert. Suggest removal

They are used before LoadX509KeyPair. When caPath and caKeyPath is == ""

you can return an empty struct and nill, for that case

at b, err := ioutil.ReadFile(caPath) on error handling should we return an empty tlsCert yet is was created or not?

I also prefer Sammy's approach. I find explicit return tls.Certificate{}, nil, nil in case of error just as fine and a bit more precise and efficient (when reading return tlsCert, caCert, nil you do not know what are actual values; but you also do not care). This is a matter of personal preference.

SammyOina · 2023-03-21T21:27:55Z

cli/consumers.go

+		},
+	},
+	{
+		Use:   "remove <thing_id> <user_auth_token>",


did you mean remove <sub_id>

cli/consumers.go

SammyOina · 2023-03-21T21:38:05Z

cli/policies.go

a lot of shared code between create and remove commands, consider refactoring

That would make it spaghetti code

perhaps with a modifier such as a flag, policy --create and --remove

Or a root command policy and sub commands create and remove

dborovcanin · 2023-05-15T08:51:48Z

certs/certs.go

@@ -30,3 +39,46 @@ type Repository interface {
 	// RetrieveBySerial retrieves a certificate for a given serial ID
 	RetrieveBySerial(ctx context.Context, ownerID, serialID string) (Cert, error)
 }
+
+func LoadCertificates(caPath, caKeyPath string) (tls.Certificate, *x509.Certificate, error) {
+	var tlsCert tls.Certificate


I also prefer Sammy's approach. I find explicit return tls.Certificate{}, nil, nil in case of error just as fine and a bit more precise and efficient (when reading return tlsCert, caCert, nil you do not know what are actual values; but you also do not care). This is a matter of personal preference.

dborovcanin · 2023-05-15T08:53:35Z

certs/certs.go

+	if _, err := os.Stat(caPath); os.IsNotExist(err) {
+		return tlsCert, caCert, err
+	}
+
+	if _, err := os.Stat(caKeyPath); os.IsNotExist(err) {
+		return tlsCert, caCert, err
+	}


Do we also care about other errors besides NotExists (such as insufficient privilege)?

dborovcanin · 2023-05-15T08:53:35Z

certs/certs.go

+	if _, err := os.Stat(caPath); os.IsNotExist(err) {
+		return tlsCert, caCert, err
+	}
+
+	if _, err := os.Stat(caKeyPath); os.IsNotExist(err) {
+		return tlsCert, caCert, err
+	}


Do we also care about other errors besides NotExists (such as insufficient privilege)?

cli/consumers.go

certs/certs.go

arvindh123 · 2023-05-16T13:24:03Z

cli/bootstrap.go

+		Use:   "get [all | <thing_id>] <user_auth_token>",
 		Short: "Get config",
-		Long:  `Get Thing Config with given ID belonging to the user identified by the given key`,
+		Long: `Get Thing Config with given ID belonging to the user identified by the given key.
+				all - lists all config
+				<thing_id> - view config of <thing_id>`,
 		Run: func(cmd *cobra.Command, args []string) {
 			if len(args) != 2 {
 				logUsage(cmd.Use)
 				return
 			}
+			pageMetadata := mfxsdk.PageMetadata{
+				Offset: uint64(Offset),
+				Limit:  uint64(Limit),
+				State:  State,
+				Name:   Name,
+			}
+			if args[0] == "all" {
+				l, err := sdk.Bootstraps(pageMetadata, args[1])
+				if err != nil {
+					logError(err)
+					return
+				}
+				logJSON(l)
+				return
+			}


mainflux-cli bootstrap get all this command will return with limit 10
How to view all things

You increase using the -l or -limit flag

pkg/sdk/go/bootstrap.go

arvindh123 · 2023-05-16T14:25:53Z

cli/bootstrap.go

@@ -98,15 +146,25 @@ var cmdBootstrap = []cobra.Command{
 		},
 	},
 	{


I tired to test the bootstrap
But I got not found

In CLI bootstrap url is not configured http://localhost
In nginx there is not route for bootstrap

The URL is provided by the -bootstrap-url or -b flag

I think for the longer form of flags, they should have double dashes --

Is this one resolved, @arvindh123 can you test and confirm?

tested , It works perfect !!

arvindh123

LGTM

dborovcanin · 2023-05-17T11:40:07Z

certs/certs_test.go

+)
+
+func TestLoadCertificates(t *testing.T) {
+


Remove the empty line.

Signed-off-by: rodneyosodo <socials@rodneyosodo.com> Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

drasko

LGTM

rodneyosodo marked this pull request as ready for review February 7, 2023 10:04

rodneyosodo requested a review from a team as a code owner February 7, 2023 10:04

dborovcanin reviewed Feb 22, 2023

View reviewed changes

rodneyosodo force-pushed the MF-1699 branch from 35e8e0a to 8518d4f Compare March 6, 2023 14:31

drasko requested changes Mar 12, 2023

View reviewed changes

rodneyosodo force-pushed the MF-1699 branch from e793251 to d722951 Compare March 20, 2023 09:25

SammyOina requested changes Mar 21, 2023

View reviewed changes

codeball-ai-writer bot added the codeball:needs-careful-review label Mar 22, 2023

rodneyosodo force-pushed the MF-1699 branch 2 times, most recently from 7d784d6 to 4a2b295 Compare April 6, 2023 07:50

rodneyosodo force-pushed the MF-1699 branch from 4a2b295 to 75ae424 Compare May 15, 2023 07:16

dborovcanin requested changes May 15, 2023

View reviewed changes

arvindh123 reviewed May 16, 2023

View reviewed changes

arvindh123 approved these changes May 17, 2023

View reviewed changes

arvindh123 previously approved these changes May 17, 2023

View reviewed changes

dborovcanin requested changes May 17, 2023

View reviewed changes

certs/certs_test.go Outdated

)

func TestLoadCertificates(t *testing.T) {

Copy link

Collaborator

dborovcanin May 17, 2023

Choose a reason for hiding this comment

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

Remove the empty line.

rodneyosodo dismissed arvindh123’s stale review via 5548552 May 17, 2023 11:54

dborovcanin approved these changes May 17, 2023

View reviewed changes

rodneyosodo and others added 11 commits May 24, 2023 10:24

initial commit update api docs

25bed7d

Signed-off-by: rodneyosodo <socials@rodneyosodo.com> Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

initial commit enrich sdk

a985b7f

Signed-off-by: rodneyosodo <socials@rodneyosodo.com> Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

initial commit enrich cli

adcb12f

Signed-off-by: rodneyosodo <socials@rodneyosodo.com> Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

add consumers test

87f5c7f

Signed-off-by: rodneyosodo <socials@rodneyosodo.com> Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

remove dead code

2434b7b

Signed-off-by: rodneyosodo <socials@rodneyosodo.com> Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

add certs sdk tests

b4ed398

Signed-off-by: rodneyosodo <socials@rodneyosodo.com> Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

change contact name

d41769b

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

fix tests

d96b5f9

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Rename Disconnect

ff72316

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Fix subscription typo

d364f76

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Fix Swagger File

53438f1

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

rodneyosodo added 3 commits May 24, 2023 10:24

Add Tests

e5ebd1c

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Change Limit to 10

ce55fb4

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

Remove empty line

797dc4d

Signed-off-by: rodneyosodo <blackd0t@protonmail.com>

dborovcanin force-pushed the MF-1699 branch from 5548552 to 797dc4d Compare May 24, 2023 08:24

drasko approved these changes May 24, 2023

View reviewed changes

drasko merged commit 408eaba into absmach:master May 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-1699 - Enrich SDK and CLI #1719

MF-1699 - Enrich SDK and CLI #1719

rodneyosodo commented Feb 7, 2023 •

edited

Loading

codecov-commenter commented Feb 7, 2023 •

edited

Loading

dborovcanin left a comment

drasko Mar 12, 2023

rodneyosodo Mar 12, 2023

SammyOina Mar 21, 2023

rodneyosodo Mar 22, 2023

SammyOina Mar 22, 2023

rodneyosodo Mar 22, 2023

dborovcanin May 15, 2023

SammyOina Mar 21, 2023

rodneyosodo Mar 22, 2023

SammyOina Mar 21, 2023

rodneyosodo Mar 22, 2023

SammyOina May 17, 2023

SammyOina May 17, 2023

dborovcanin May 15, 2023

dborovcanin May 15, 2023

dborovcanin May 15, 2023

arvindh123 May 16, 2023

rodneyosodo May 17, 2023

arvindh123 May 16, 2023

rodneyosodo May 17, 2023

SammyOina May 17, 2023

SammyOina May 17, 2023

dborovcanin May 17, 2023

arvindh123 May 17, 2023

arvindh123 left a comment

dborovcanin May 17, 2023

drasko left a comment

@@ @@ -98,15 +146,25 @@ var cmdBootstrap = []cobra.Command{ @@
               		},
               	},
               	{

MF-1699 - Enrich SDK and CLI #1719

MF-1699 - Enrich SDK and CLI #1719

Conversation

rodneyosodo commented Feb 7, 2023 • 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 Feb 7, 2023 • edited Loading

Codecov Report

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

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

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

arvindh123 left a 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

rodneyosodo commented Feb 7, 2023 •

edited

Loading

codecov-commenter commented Feb 7, 2023 •

edited

Loading