-
Notifications
You must be signed in to change notification settings - Fork 671
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 - Refactor single-user mode #1471
Conversation
Signed-off-by: dusanb94 <[email protected]>
Signed-off-by: dusanb94 <[email protected]>
cmd/twins/main.go
Outdated
@@ -23,7 +23,7 @@ import ( | |||
"github.com/mainflux/mainflux/pkg/messaging" | |||
"github.com/mainflux/mainflux/pkg/messaging/nats" | |||
"github.com/mainflux/mainflux/pkg/uuid" | |||
localusers "github.com/mainflux/mainflux/things/users" | |||
localusers "github.com/mainflux/mainflux/things/auth" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only cocncern is should this one be called auth
, because again when we take a look at things
folder in a couple of months and when we see auth
- will we know it is a Single-User Mode related module? I was confused like this with previous naming, which was user
.
So just one thing to conside - should it be something like singleuser
(or something like this, self-documenting), or we leave auth
(I am fine with auth
also, just challenging the solution a bit).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually spent some time thinking about the naming and I decided to go with auth
and single_user
for go files. The reason is that this is implementation of auth
for single_user
. It is not ideal, but I find it a bit less confusing than users
. Also, standalone
sounds a bit better to me than single_user
, but I did not want to rename env vars to provide backward compatibility. @drasko What do you think about that auth
and standalone
?
@@ -101,6 +101,10 @@ $GOBIN/mainflux-things | |||
|
|||
Setting `MF_THINGS_CA_CERTS` expects a file in PEM format of trusted CAs. This will enable TLS against the Users gRPC endpoint trusting only those CAs that are provided. | |||
|
|||
In constrained environments, sometimes it makes sense to run Things service as a standalone to reduce network traffic and simplify deployment. This means that Things service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a more detailed documentation in the mainflux/docs
repo is needed, with detailed explication how this single-user mode is deployed (Dockerfile) and used.
I.e. - there should be a dedicated chapter (or subsection in Authorization) for Single-User Mode, so that people see and understand that this feature exists (otherwise, it is unnoticed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but one paragraph is sufficient for this, since you just set the environment variables and one example how things
api is used then, additionally advise to remove auth service from compose when using this mode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @mteodor, I'd say this is good enough for service README. I'll add suggestion for removing auth
from compose and we can add more details in a separate section in docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed single-user
to standalone
, please review.
a3c1b1a
to
42ca807
Compare
Signed-off-by: dusanb94 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Codecov Report
@@ Coverage Diff @@
## master #1471 +/- ##
=======================================
Coverage 70.92% 70.92%
=======================================
Files 123 123
Lines 9565 9565
=======================================
Hits 6784 6784
Misses 2254 2254
Partials 527 527
Continue to review full report at Codecov.
|
* Refactor single user mode Signed-off-by: dusanb94 <[email protected]> * Fix Twins dependency Signed-off-by: dusanb94 <[email protected]> * Rename `single-user` to `standalone` Signed-off-by: dusanb94 <[email protected]> Co-authored-by: Drasko DRASKOVIC <[email protected]>
* Refactor single user mode Signed-off-by: dusanb94 <[email protected]> * Fix Twins dependency Signed-off-by: dusanb94 <[email protected]> * Rename `single-user` to `standalone` Signed-off-by: dusanb94 <[email protected]> Co-authored-by: Drasko DRASKOVIC <[email protected]>
* Refactor single user mode Signed-off-by: dusanb94 <[email protected]> * Fix Twins dependency Signed-off-by: dusanb94 <[email protected]> * Rename `single-user` to `standalone` Signed-off-by: dusanb94 <[email protected]> Co-authored-by: Drasko DRASKOVIC <[email protected]>
* Refactor single user mode Signed-off-by: dusanb94 <[email protected]> * Fix Twins dependency Signed-off-by: dusanb94 <[email protected]> * Rename `single-user` to `standalone` Signed-off-by: dusanb94 <[email protected]> Co-authored-by: Drasko DRASKOVIC <[email protected]>
Signed-off-by: dusanb94 <[email protected]
What does this do?
This pull request refactors single-user mode for Things service.
Which issue(s) does this PR fix/relate to?
There is no such issue.
List any changes that modify/break current functionality
There are no such changes.
Have you included tests for your changes?
Yes.
Did you document any new/modified functionality?
Yes. Docs explaining single-user mode are added.
Notes
N/A