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

Refactored usage of service clients to be compatible with latest contracts changes. #447

Merged
merged 3 commits into from
Apr 8, 2020
Merged

Refactored usage of service clients to be compatible with latest contracts changes. #447

merged 3 commits into from
Apr 8, 2020

Conversation

brandonforster
Copy link
Contributor

@brandonforster brandonforster commented Feb 13, 2020 •
edited
Loading

Fixes #446
Fixes #445

Signed-off-by: Brandon Forster me@brandonforster.com

@brandonforster
Copy link
Contributor Author

@Nitu-coder try this changeset. I believe it will address your issue.

@codecov-io
Copy link

codecov-io commented Feb 13, 2020 •
edited
Loading

Codecov Report

Merging #447 into master will decrease coverage by 0.44%.
The diff coverage is 4.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #447      +/-   ##
==========================================
- Coverage   56.21%   55.77%   -0.45%     
==========================================
  Files          24       24              
  Lines        2396     2415      +19     
==========================================
  Hits         1347     1347              
- Misses        951      970      +19     
  Partials       98       98
Impacted Files Coverage Δ
internal/clients/init.go 7.29% <0.00%> (-1.18%) ⬇️
internal/transformer/transformresult.go 66.51% <0.00%> (ø)
internal/common/utils.go 19.87% <20.00%> (ø)
internal/cache/init.go 59.09% <100.00%> (ø)

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 79a40a3...ee9a22e. Read the comment docs.

lenny-goodell
lenny-goodell reviewed Feb 13, 2020
View reviewed changes
Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

one question, otherwise look good.

managedprofiles.go Outdated Show resolved Hide resolved
cloudxxx8
cloudxxx8 requested changes Feb 14, 2020
View reviewed changes
Copy link
Member

@cloudxxx8 cloudxxx8 left a comment

Choose a reason for hiding this comment

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

Thanks for the effort on this PR. Please see whether my review comments are reasonable.

internal/mock/mock_eventclient.go Outdated Show resolved Hide resolved
internal/endpoint/endpoint.go Outdated Show resolved Hide resolved
internal/urlclient/factory.go Outdated Show resolved Hide resolved
internal/endpoint/endpoint.go Show resolved Hide resolved
internal/endpoint/endpoint.go Show resolved Hide resolved
internal/endpoint/endpoint.go Show resolved Hide resolved
internal/clients/init.go Outdated Show resolved Hide resolved
@brandonforster
Copy link
Contributor Author

@cloudxxx8 Rebased and updated, please take a look.

cloudxxx8
cloudxxx8 reviewed Mar 29, 2020
View reviewed changes
internal/clients/init.go Outdated Show resolved Hide resolved
@brandonforster
Copy link
Contributor Author

This PR is now based on #432. #432 MUST be merged before this PR.

@lenny-goodell
Copy link
Member

This PR is now based on #432. #432 MUST be merged before this PR.

This makes your PR include all the changes from #432 which makes it impossible to review just your changes. I will re-review once #432 has been merged and can review just your changes.

@brandonforster
Copy link
Contributor Author

@lenny-intel basically yes, but if you wanted to see my diff, that is at 5e5fdef

@brandonforster
Copy link
Contributor Author

Now that #432 is merged, this PR is ready for review.

@lenny-intel @cloudxxx8

lenny-goodell
lenny-goodell requested changes Apr 6, 2020
View reviewed changes
Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

Just one very minor comment to fix.

internal/urlclient/factory.go Outdated Show resolved Hide resolved
@brandonforster
#446: Refactored usage of service clients to be compatible with lates…
a50d6b0 
…t contracts changes.

Signed-off-by: Brandon Forster <me@brandonforster.com>
lenny-goodell
lenny-goodell previously approved these changes Apr 6, 2020
View reviewed changes
Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

LGTM

chr1shung
chr1shung reviewed Apr 7, 2020
View reviewed changes
Copy link

@chr1shung chr1shung left a comment

Choose a reason for hiding this comment

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

LGTM overall, need some clarifications on the behavior with -r argument

internal/urlclient/factory.go Outdated Show resolved Hide resolved
internal/clients/init.go Outdated Show resolved Hide resolved
internal/endpoint/endpoint.go
defer e.wg.Done()

// run fetchURL once before looping so we get the first check before the first timer interval
e.fetchURL(ch)
Copy link

Choose a reason for hiding this comment

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

Maybe it's better to call e.wg.Done() after first check ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using defer instead of calling it explicitly allows it to run even when an error occurs. If we called it under the ctx.Done() case, it would only finish on the happy path. This way, it finishes whenever the routine ends, whether that's because it was successful or because it failed.

Copy link

@chr1shung chr1shung Apr 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

my suggestion is a followup of the above discussion: if we keep waitGroup.wait() we should call wg.Done() immediately after first check so that the main thread won't be blocked and then we can still monitor the URL change in the background

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should we do in the error case? Never call wg.Done()?

@brandonforster
#446: Updated to use context passed from bootstrap.
72b46d5 
Signed-off-by: Brandon Forster <me@brandonforster.com>
lenny-goodell
lenny-goodell requested changes Apr 7, 2020
View reviewed changes
internal/clients/init.go Outdated Show resolved Hide resolved
@brandonforster
#446: Removed unecessary call to wg.Wait
ee9a22e 
Signed-off-by: Brandon Forster <me@brandonforster.com>
lenny-goodell
lenny-goodell approved these changes Apr 7, 2020
View reviewed changes
Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

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

LGTM

@cloudxxx8 cloudxxx8 merged commit 44a3efc into edgexfoundry:master Apr 8, 2020
@brandonforster brandonforster deleted the issue_446 branch April 8, 2020 13:21
@cloudxxx8 cloudxxx8 mentioned this pull request Apr 9, 2020
Closed
@chr1shung chr1shung mentioned this pull request Apr 9, 2020
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cloudxxx8 cloudxxx8 cloudxxx8 approved these changes

@lenny-goodell lenny-goodell lenny-goodell approved these changes

@FelixTing FelixTing Awaiting requested review from FelixTing

@michaelestrin michaelestrin Awaiting requested review from michaelestrin

@chr1shung chr1shung Awaiting requested review from chr1shung

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

Successfully merging this pull request may close these issues.

Integrate go-mod-contracts changes to service clients Error in installing device-sdk-go via command line
6 participants
@brandonforster @codecov-io @lenny-goodell @michaelestrin @cloudxxx8 @chr1shung