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.

Sign up for GitHub

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

Jump to bottom

NOISSUE - Fix Postgres logs in Things service #734

Merged
merged 10 commits into from
May 10, 2019
Merged

NOISSUE - Fix Postgres logs in Things service #734

merged 10 commits into from
May 10, 2019

Conversation

manuio
Copy link
Contributor

@manuio manuio commented May 3, 2019

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

@manuio
NOISSUE - Fix Postgres logs in Things service
b8d3fe2 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@codecov-io
Copy link

codecov-io commented May 3, 2019 •
edited
Loading

Codecov Report

Merging #734 into master will increase coverage by 0.21%.
The diff coverage is 57.89%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #734      +/-   ##
=========================================
+ Coverage   85.08%   85.3%   +0.21%     
=========================================
  Files          66      66              
  Lines        4232    4220      -12     
=========================================
- Hits         3601    3600       -1     
+ Misses        429     416      -13     
- Partials      202     204       +2
Impacted Files Coverage Δ
things/things.go 100% <ø> (ø) ⬆️
things/service.go 92.02% <100%> (ø) ⬆️
things/postgres/channels.go 78.94% <52.94%> (+2.17%) ⬆️
things/postgres/things.go 78.91% <52.94%> (+2.19%) ⬆️

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 5e5fe88...d408e3c. Read the comment docs.

anovakovic01
anovakovic01 suggested changes May 3, 2019
View reviewed changes
Copy link
Contributor

@anovakovic01 anovakovic01 left a comment

Choose a reason for hiding this comment

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

I think that you'll need to update http API layer in order for this to work (I don't see that you've added error handling for this method there).

things/postgres/channels.go Outdated Show resolved Hide resolved
things/postgres/channels.go Outdated Show resolved Hide resolved
things/postgres/channels.go Outdated Show resolved Hide resolved
things/postgres/channels.go Outdated Show resolved Hide resolved
things/postgres/channels.go Outdated Show resolved Hide resolved
things/postgres/channels_test.go Outdated Show resolved Hide resolved
things/postgres/channels_test.go Outdated Show resolved Hide resolved
things/postgres/setup_test.go Outdated Show resolved Hide resolved
things/postgres/things_test.go Outdated Show resolved Hide resolved
manuio added 2 commits May 3, 2019 17:32
@manuio
Fix reviews
dd463ad 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio
Fix reviews
c89ca99 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
drasko
drasko previously approved these changes May 7, 2019
View reviewed changes
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

anovakovic01
anovakovic01 suggested changes May 7, 2019
View reviewed changes
things/postgres/channels.go Outdated Show resolved Hide resolved
things/postgres/channels.go Show resolved Hide resolved
things/postgres/channels_test.go Outdated Show resolved Hide resolved
things/postgres/channels_test.go Outdated Show resolved Hide resolved
things/postgres/things.go Outdated Show resolved Hide resolved
things/postgres/things_test.go Outdated Show resolved Hide resolved
things/postgres/things_test.go Outdated Show resolved Hide resolved
@manuio
Fix reviews
5f94be2 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
anovakovic01
anovakovic01 suggested changes May 8, 2019
View reviewed changes
things/postgres/channels_test.go Outdated Show resolved Hide resolved
manuio added 2 commits May 9, 2019 15:44
@manuio
Fix reviews
866a1fc 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio
Merge branch 'things-logs' of https://www.github.com/manuio/mainflux …
7fe551d 
…into things-logs
drasko
drasko reviewed May 9, 2019
View reviewed changes
things/postgres/things.go
func (tr thingRepository) RetrieveByChannel(owner, channel string, offset, limit uint64) things.ThingsPage {
func (tr thingRepository) RetrieveByChannel(owner, channel string, offset, limit uint64) (things.ThingsPage, error) {
// Verify if UUID format is valid to avoid internal Postgres error
if _, err := uuid.FromString(channel); 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.

Why are we doing this UUID validation here and not at transport layer, or at some earlier stages?

Copy link
Contributor

Choose a reason for hiding this comment

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

@anovakovic01 @nmarcetic @dusanb94 please also comment on this question ^

Copy link
Contributor

Choose a reason for hiding this comment

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

Because if we ever change ID format (because of DB change or something else) we only have to change repository implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, thanks for clarification.

drasko
drasko previously approved these changes May 9, 2019
View reviewed changes
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

dborovcanin
dborovcanin previously approved these changes May 9, 2019
View reviewed changes
@manuio
Fix reviews
803d337 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
@manuio manuio dismissed stale reviews from dborovcanin and drasko via 803d337 May 10, 2019 14:55
anovakovic01
anovakovic01 previously approved these changes May 10, 2019
View reviewed changes
Copy link
Contributor

@anovakovic01 anovakovic01 left a comment

Choose a reason for hiding this comment

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

LGTM!

manuio and others added 2 commits May 10, 2019 16:57
@manuio
Merge branch 'master' into things-logs
7e126a5
@manuio
Fix uuid package
d408e3c 
Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
anovakovic01
anovakovic01 approved these changes May 10, 2019
View reviewed changes
Copy link
Contributor

@anovakovic01 anovakovic01 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

drasko
drasko approved these changes May 10, 2019
View reviewed changes
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 d2fc379 into absmach:master May 10, 2019
@manuio manuio deleted the things-logs branch May 10, 2019 21:53
dborovcanin pushed a commit to dborovcanin/magistrala that referenced this pull request May 13, 2019
@manuio @dborovcanin
NOISSUE - Fix Postgres logs in Things service (absmach#734)
e11ec79 
* NOISSUE - Fix Postgres logs in Things service

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix uuid package

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
dborovcanin pushed a commit to dborovcanin/magistrala that referenced this pull request May 13, 2019
@manuio @dborovcanin
NOISSUE - Fix Postgres logs in Things service (absmach#734)
fec105f 
* NOISSUE - Fix Postgres logs in Things service

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix uuid package

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
davide83 pushed a commit to davide83/mainflux that referenced this pull request May 13, 2019
@manuio @davide83
NOISSUE - Fix Postgres logs in Things service (absmach#734)
9e5445e 
* NOISSUE - Fix Postgres logs in Things service

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix uuid package

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
rugwirobaker pushed a commit to rugwirobaker/mainflux that referenced this pull request Jun 26, 2019
@manuio @rugwirobaker
NOISSUE - Fix Postgres logs in Things service (absmach#734)
7fcfd41 
* NOISSUE - Fix Postgres logs in Things service

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix uuid package

Signed-off-by: Manuel Imperiale <manuel.imperiale@gmail.com>
manuio added a commit that referenced this pull request Oct 12, 2020
@manuio @drasko
NOISSUE - Fix Postgres logs in Things service (#734)
98d5426 
* NOISSUE - Fix Postgres logs in Things service

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix reviews

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

* Fix uuid package

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
Reviewers

@drasko drasko drasko approved these changes

@anovakovic01 anovakovic01 anovakovic01 approved these changes

@dborovcanin dborovcanin dborovcanin left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@manuio @codecov-io @drasko @anovakovic01 @dborovcanin