NOISSUE - Certs service refactor #1369
Conversation
There was a problem hiding this comment.
Is this one duplicate of https://github.com/mainflux/mainflux/pull/1368?
|
no not a duplicate, notice the description |
Codecov Report
@@ Coverage Diff @@
## master #1369 +/- ##
==========================================
+ Coverage 67.56% 67.67% +0.10%
==========================================
Files 121 122 +1
Lines 8420 8467 +47
==========================================
+ Hits 5689 5730 +41
- Misses 2215 2218 +3
- Partials 516 519 +3
Continue to review full report at Codecov.
|
certs/api/logging.go
Outdated
| defer func(begin time.Time) { | ||
| message := fmt.Sprintf("Method list_certs for token: %s took %s to complete", token, time.Since(begin)) | ||
| message := fmt.Sprintf("Method list_certs for token: %s and thing ID:%s took %s to complete", token, thingID, time.Since(begin)) |
There was a problem hiding this comment.
Add a white space after : or it will be hard to read. Also, I think that we don't use capital letters in logs.
certs/certs.go
Outdated
| @@ -19,7 +19,7 @@ type Repository interface { | |||
| Save(ctx context.Context, cert Cert) (string, error) | |||
|
|
|||
| // RetrieveAll retrieve all issued certificates for given owner | |||
There was a problem hiding this comment.
This comment is still correct?
certs/mocks/things.go
Outdated
| conns := make(map[string][]string) | ||
| for k, v := range svc.connections { | ||
| i := findIndex(v, id) | ||
| if i != -1 { |
There was a problem hiding this comment.
What are all these magic numbers? Either add comments or move them to constants (or both).
certs/mocks/things.go
Outdated
| } | ||
|
|
||
| var tmp []string | ||
| if i != len(ids)-2 { |
There was a problem hiding this comment.
Magic numbers, add comments or use constants.
certs/mocks/things.go
Outdated
|
|
||
| ids := svc.connections[chanID] | ||
| i := 0 | ||
| for _, t := range ids { |
There was a problem hiding this comment.
What is this doing? Probably some comments are needed here.
Also, can this search be improved?
There was a problem hiding this comment.
this file things.go is copied from other service, there is opened issue for refactoring mocks so that we don't copy in each service same code, so this could be part of that PR https://github.com/mainflux/mainflux/issues/1382 as basically it is not related to changes in this PR
certs/api/endpoint.go
Outdated
| @@ -18,9 +18,15 @@ func issueCert(svc certs.Service) endpoint.Endpoint { | |||
| } | |||
| res, err := svc.IssueCert(ctx, req.token, req.ThingID, req.Valid, req.KeyBits, req.KeyType) | |||
| if err != nil { | |||
| return certsResponse{Error: err.Error()}, nil | |||
| return thingCertsRes{Error: err.Error()}, nil | |||
There was a problem hiding this comment.
What else it can be than things certs?
certs/api/transport.go
Outdated
| @@ -51,14 +51,14 @@ func MakeHandler(svc certs.Service) http.Handler { | |||
| opts..., | |||
| )) | |||
|
|
|||
| r.Get("/certs", kithttp.NewServer( | |||
| r.Get("/certs/:id", kithttp.NewServer( | |||
certs/pki/vault.go
Outdated
| // ErrNotImplemented indicate that method called is not implemented | ||
| ErrNotImplemented = errors.New("method not implemented for certs") | ||
|
|
||
| // ErrMissingCACertificate indicates missing CA certificate. |
There was a problem hiding this comment.
Please remove punctuation to align with the other comments.
certs/service.go
Outdated
| errFailedToRemoveCertFromDB = errors.New("failed to remove cert serial from db") | ||
| errFailedCertCreation = errors.New("failed to create client certificate") | ||
| errFailedCertRevocation = errors.New("failed to revoke certificate") | ||
| // ErrFailedCertCreation failed to create certificate. |
There was a problem hiding this comment.
You can put a single comment explanation above this var block to avoid writing unnecessary comments just to satisfy linter.
certs/service_test.go
Outdated
| ) | ||
|
|
||
| func newService(tokens map[string]string) (certs.Service, error) { | ||
|
|
There was a problem hiding this comment.
Please remove the empty line.
certs/certs.go
Outdated
| @@ -18,8 +18,8 @@ type Repository interface { | |||
| // Save saves cert for thing into database | |||
| Save(ctx context.Context, cert Cert) (string, error) | |||
|
|
|||
| // RetrieveAll retrieve all issued certificates for given owner | |||
| RetrieveAll(ctx context.Context, ownerID string, offset, limit uint64) (Page, error) | |||
| // RetrieveAll retrieve all issued certificates for given owner adn thing id | |||
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
certs/api/endpoint.go
Outdated
| @@ -18,9 +18,15 @@ func issueCert(svc certs.Service) endpoint.Endpoint { | |||
| } | |||
| res, err := svc.IssueCert(ctx, req.token, req.ThingID, req.Valid, req.KeyBits, req.KeyType) | |||
| if err != nil { | |||
| return certsResponse{Error: err.Error()}, nil | |||
| return certsRes{Error: err.Error()}, nil | |||
There was a problem hiding this comment.
Why both return error and error inside response?
| @@ -171,17 +150,17 @@ func (cs *certsService) RevokeCert(ctx context.Context, token, thingID string) ( | |||
| } | |||
| thing, err := cs.sdk.Thing(thingID, token) | |||
| if err != nil { | |||
| return revoke, errors.Wrap(errFailedCertRevocation, err) | |||
| return revoke, errors.Wrap(ErrFailedCertRevocation, err) | |||
There was a problem hiding this comment.
Revoke seems to be a simple wrapper around time.Time. Since it's unlikely its content will ever change, this wrapper seems to be unnecessary; you can return time directly.
There was a problem hiding this comment.
more or less I copied response from vault https://www.vaultproject.io/api/secret/pki#sample-request-14
There was a problem hiding this comment.
You can (and need) to wrap time in API layer, but there is no need to wrap it in the service method.
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
certs/pki/vault.go
Outdated
| errFailedVaultCertIssue = errors.New("failed to issue vault certificate") | ||
| errFailedCertDecoding = errors.New("failed to decode response from vault service") | ||
| ) | ||
|
|
||
| type Revoke struct { |
There was a problem hiding this comment.
Can this struct be removed?
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
certs/api/responses.go
Outdated
| type certsRes struct { | ||
| ID string `json:"thing_id"` | ||
| Cert string `json:"thing_cert"` | ||
| Key string `json:"thing_cert_key"` | ||
| Serial string `json:"thing_cert_serial"` |
There was a problem hiding this comment.
tags should follow field name
Signed-off-by: Mirko Teodorovic <mirko.teodorovic@gmail.com>
What does this do?
Some code refactoring, fix endpoints and openapi,
create another implementation of PKI agent along with Vault wrapper agent implementation ( vault.go)
list certificates by thing id
List any changes that modify/break current functionality
Have you included tests for your changes?
Did you document any new/modified functionality?
Notes