-
Notifications
You must be signed in to change notification settings - Fork 17
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
MF-20 - Refactor service #41
Conversation
@@ -16,27 +16,40 @@ const ( | |||
device = "device" |
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.
Why do we have folder services, and in it service.go
? Are these local services on the gateway?
It is a little confusing, because we have service.go
which relates to agent and then services
which relate to local services...
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.
But I currently have no better idea for naming.
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.
can we rename service.go
to agent.go
or maybe services/info.go
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.
Well... I am more bothered about the folder name services
. I think we should rename service.go
into agent.go
for sure, and we can leave this services
for now, but we need to document it somewhere - maybe add one README.md in the agent
or these services
folder...
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.
for the folder maybe info
, or stats
so we have info/service.go
or maybe since it is basically only for heartbeat to call it heartbeat/service.go
Signed-off-by: Mirko Teodorovic <[email protected]>
Signed-off-by: Mirko Teodorovic <[email protected]>
Signed-off-by: Mirko Teodorovic <[email protected]>
Signed-off-by: Mirko Teodorovic <[email protected]>
Signed-off-by: Mirko Teodorovic <[email protected]>
538bb41
to
79395d9
Compare
pkg/agent/heartbeat.go
Outdated
@@ -16,27 +16,40 @@ const ( | |||
device = "device" | |||
) | |||
|
|||
type Service struct { | |||
type svc struct { | |||
name string |
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.
This looks very similar to Info{} - maybe one struct should contain another?
Signed-off-by: Mirko Teodorovic <[email protected]>
Signed-off-by: Mirko Teodorovic <[email protected]>
Signed-off-by: Mirko Teodorovic <[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.
Looks good, a small nitpick left.
pkg/agent/heartbeat.go
Outdated
// Whenservice doesnt send heartbeat for some time gets marked offline | ||
type svc struct { | ||
info Info | ||
|
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.
No need for this whie line IMHO.
Signed-off-by: Mirko Teodorovic <[email protected]>
for { | ||
select { | ||
case <-s.ticker.C: | ||
// TODO - we can disable ticker when the status gets OFFLINE |
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.
Can you create an issue for this please?
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.
done #42
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
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
closes #20