-
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
add location endpoint #2481
add location endpoint #2481
Conversation
- add location endpoint to get this file info
pkg/registrar/register.go
Outdated
@@ -231,3 +237,13 @@ func ensureTwin(ctx context.Context, substrateGateway *stubs.SubstrateGatewayStu | |||
|
|||
return twinID, nil | |||
} | |||
|
|||
func writeLocationOnFile(loc geoip.Location, filepath string) error { | |||
file, err := os.OpenFile(filepath, os.O_RDWR|os.O_CREATE, 0644) |
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.
is it possible for the file to already exists?
If possible, what is the default behavior? Is truncate or append?
pkg/zos_api/location.go
Outdated
) | ||
|
||
func (g *ZosAPI) locationGet(ctx context.Context, payload []byte) (interface{}, error) { | ||
if _, err := os.Stat(geoip.LocationFile); err != nil { |
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.
is it needed?
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 should be addressed differently, I don't see a problem calling that function from the api and returning its result to the user
pkg/zos_api/location.go
Outdated
var loc geoip.Location | ||
err = json.NewDecoder(f).Decode(&loc) | ||
return loc, err | ||
return geoip.Fetch() |
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.
at this point you can add caching if needed
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, the node location should be updated like once per day and then cached for full day before it's fetched again.
i will draft until we finish releasing today |
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.
Thank @Omarabdul3ziz for implementing this. It looks good to me. It's yet a good idea to cache the location of the node to avoid un-necessary load on the location service as per @xmonader comment
Description
Related Issues
Checklist