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 - Rename package aliases uuidProvider into uuid #1323

Merged
merged 13 commits into from
Jan 17, 2021
Merged

NOISSUE - Rename package aliases uuidProvider into uuid #1323

merged 13 commits into from
Jan 17, 2021

Conversation

manuio
Copy link
Contributor

@manuio manuio commented Jan 11, 2021

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

@manuio manuio requested a review from a team as a code owner January 11, 2021 17:20
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.

If I understand well, this is to remove dependency against Mainflux UUID provider?

However, I am afraid that whoever starts removing Mainflux UUID provider and replacing it with the ULID will completely forget and miss this file, as it will be no longer using it.
his is why I prefer keeping dependency, and reminding that the more general task should be undertaken - overall switch to ULID.

mteodor
mteodor previously approved these changes Jan 12, 2021
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@codecov-io
Copy link

codecov-io commented Jan 12, 2021

Codecov Report

Merging #1323 (0b1d13c) into master (0516fe2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1323   +/-   ##
=======================================
  Coverage   59.89%   59.90%           
=======================================
  Files         113      113           
  Lines        8835     8836    +1     
=======================================
+ Hits         5292     5293    +1     
  Misses       3081     3081           
  Partials      462      462           
Impacted Files Coverage Δ
auth/service.go 43.96% <100.00%> (ø)
things/service.go 54.54% <100.00%> (ø)
twins/service.go 69.38% <100.00%> (ø)
users/service.go 66.66% <100.00%> (+0.24%) ⬆️

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 0516fe2...0b1d13c. Read the comment docs.

@@ -38,13 +38,13 @@ var (
func TestReadSenml(t *testing.T) {
writer := pwriter.New(db)

chanID, err := uuidProvider.New().ID()
chanID, err := uuid.New().ID()
Copy link
Collaborator

@dborovcanin dborovcanin Jan 12, 2021

Choose a reason for hiding this comment

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

There is no need to instantiate a new ID provider for each ID. You can create one provider and use it for all IDs in this test. Check all the other places in tests where you have multiple instances.

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>
"github.com/stretchr/testify/require"
)

const (
Copy link
Collaborator

Choose a reason for hiding this comment

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

const email = "user-save@example.com"

readers/cassandra/messages_test.go Show resolved Hide resolved
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
mteodor
mteodor previously approved these changes Jan 13, 2021
users/service.go Outdated
hasher Hasher
email Emailer
auth mainflux.AuthServiceClient
uuidProvider mainflux.IDProvider
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not necessarily a UUID provider (we'll switch to ULID soon) and the underlying format should be transparent to the Users service (that's one of the reasons we use the interface), so idp or idProvider sounds like a better naming.

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, let's follow interface naming.

users/service.go Outdated
@@ -186,7 +187,7 @@ func (svc usersService) Register(ctx context.Context, user User) (string, error)
return "", errors.Wrap(ErrMalformedEntity, err)
}
user.Password = hash
uid, err := uuidProvider.New().ID()
uid, err := svc.uuidProvider.ID()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm glad you fixed this, it was a horrible mistake.

@@ -284,7 +285,9 @@ func newService(db *sqlx.DB, tracer opentracing.Tracer, auth mainflux.AuthServic
logger.Error(fmt.Sprintf("Failed to configure e-mailing util: %s", err.Error()))
}

svc := users.New(userRepo, groupRepo, hasher, auth, emailer)
uuidProvider := uuid.New()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we are using hasher as an instance of the hasher (bcrypt), we should do the same for the ID provider. We don't really care what's the underlying implementation.

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
blokovi
blokovi previously approved these changes Jan 14, 2021
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
dborovcanin
dborovcanin previously approved these changes Jan 14, 2021
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 0f856f5 into absmach:master Jan 17, 2021
@manuio manuio deleted the uuid branch January 18, 2021 09:09
fbugarski pushed a commit to fbugarski/mainflux that referenced this pull request Mar 8, 2021
* NOISSUE - Rename pkg aliases uuidProvider into uuid and fix authn typo

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

* Add missing error checks

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

* Use global uuidProvider

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

* Use expTime globally

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

* Fix user uuid provider

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

* Fix review

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

* Use idProvider name

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

* Use idProvider instead of uuidProvider

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

* Use idProvider instead of uuidProvider

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
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