Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NOISSUE - Add view and list serials endpoints in certs service #1483

Merged
merged 47 commits into from
Jan 4, 2022
Merged

NOISSUE - Add view and list serials endpoints in certs service #1483

merged 47 commits into from
Jan 4, 2022

Conversation

manuio
Copy link
Contributor

@manuio manuio commented Oct 26, 2021

Signed-off-by: Manuel Imperiale manuel.imperiale@gmail.com

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio manuio requested a review from a team as a code owner October 26, 2021 08:07
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2021

Codecov Report

Merging #1483 (2b92826) into master (dd7d52e) will decrease coverage by 0.02%.
The diff coverage is 45.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1483      +/-   ##
==========================================
- Coverage   67.59%   67.56%   -0.03%     
==========================================
  Files         133      133              
  Lines       10933    10979      +46     
==========================================
+ Hits         7390     7418      +28     
- Misses       2938     2953      +15     
- Partials      605      608       +3     
Impacted Files Coverage Δ
certs/postgres/certs.go 0.00% <0.00%> (ø)
certs/service.go 85.71% <77.27%> (-2.75%) ⬇️
things/service.go 66.07% <0.00%> (+0.71%) ⬆️

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 dd7d52e...2b92826. Read the comment docs.

@@ -20,12 +20,14 @@ func issueCert(svc certs.Service) endpoint.Endpoint {
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @manuio for tightening this service, this change is not related to the stuff you did but if you could change req.Valid to req.TTL as that would be more appropriate ( and days_valid to TTL in openapi doc)

Copy link
Contributor

Choose a reason for hiding this comment

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

I like this proposition.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure but I think that this remark was previous to the renaming of days_valid into hours_valid. IMO it's better to use hours_valid, it's self documenting. ttl is a Vault therminology.

Copy link
Contributor

Choose a reason for hiding this comment

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

TTL is not a Vault terminology.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that hours_valid, while a little clumsy, is more self-documenting. However, since we are using it as a string, we can include a time determinant such as "h", "m" or "s", so TTL is probably a better naming. Also, since we don't control the aforementioned time determinant, hours in the name is actually misleading.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. What I mean is that we are using TTL in the pki layer but we don't necessaraly have to follow this therminology for the HTTP API. However we are using KeyBytes and KeyType so maybe I can change it.

Copy link
Contributor Author

@manuio manuio Jan 3, 2022

Choose a reason for hiding this comment

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

@drasko @dusanb94 ok, let me rename then

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
mteodor
mteodor previously approved these changes Oct 26, 2021
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

This PR looks OK to me, but I would let @dusanb94 review and approve before we merge.

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
…ertion

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Copy link
Contributor

@mteodor mteodor left a comment

Choose a reason for hiding this comment

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

LGTM

mteodor
mteodor previously approved these changes Nov 4, 2021
@drasko
Copy link
Contributor

drasko commented Nov 5, 2021

I think there should be a documentation added for this

Copy link
Collaborator

@dborovcanin dborovcanin left a comment

Choose a reason for hiding this comment

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

Please add tests for new service methods.

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
mteodor
mteodor previously approved these changes Nov 18, 2021
blokovi
blokovi previously approved these changes Nov 18, 2021
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
manuio and others added 14 commits December 14, 2021 12:25
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
…ertion

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio
Copy link
Contributor Author

manuio commented Dec 21, 2021

@drasko @dusanb94 Can you review please?

api/certs.yml Outdated Show resolved Hide resolved
@@ -20,12 +20,14 @@ func issueCert(svc certs.Service) endpoint.Endpoint {
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this resolved?

c.certsByThingID[cert.OwnerID] = map[string][]certs.Cert{
cert.ThingID: []certs.Cert{crt},
}
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please avoid using else.

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio manuio dismissed stale reviews from blokovi and mteodor via 95033bf January 3, 2022 12:45
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Copy link
Contributor

@drasko drasko left a comment

Choose a reason for hiding this comment

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

LGTM

@drasko drasko merged commit aa014c2 into absmach:master Jan 4, 2022
bioinformatx pushed a commit to bioinformatx/mainflux that referenced this pull request Jan 28, 2022
…ch#1483)

* NOISSUE - Add view and list serials endpoints in certs service

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

* Fix vault-unseal.sh script

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

* Rename Cert field days_valid into hours_valid

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

* Fix provision service

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

* Use ownerID, rename daysValid -> hoursValid

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

* Add key_type to api

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

* Fix tabulation

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

* Add expiration date in view response

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

* Rename HoursValid -> Expiration and remove unecessary expiration convertion

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

* Add ListSerials tests and fix mocks

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

* Fix RetrieveByThing count

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

* Add ViewCert tests

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

* Add missing error check

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

* Simplify API

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

* Revert Makefile

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

* Fix typo

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

* NOISSUE - Add view and list serials endpoints in certs service

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

* Fix vault-unseal.sh script

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

* Rename Cert field days_valid into hours_valid

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

* Fix provision service

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

* Use ownerID, rename daysValid -> hoursValid

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

* Add key_type to api

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

* Fix tabulation

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

* Add expiration date in view response

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

* Rename HoursValid -> Expiration and remove unecessary expiration convertion

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

* Add ListSerials tests and fix mocks

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

* Fix RetrieveByThing count

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

* Add ViewCert tests

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

* Add missing error check

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

* Simplify API

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

* Revert Makefile

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

* Rm if else

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

* Rename HoursValid -> TTL

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

* revert typo

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

* revert typo

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

* Rename hoursValid -> ttl

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
Signed-off-by: skovacevic <stefan.kovacevic@mainflux.io>
@manuio manuio deleted the certs branch February 23, 2022 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants