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

MYST 532 Tequilapi new Location Endpoint #235

Merged
merged 9 commits into from May 11, 2018

Conversation

Projects
None yet
5 participants
@interro
Copy link
Contributor

commented May 8, 2018

No description provided.

@interro interro requested review from donce, tadovas and Waldz as code owners May 8, 2018

@@ -38,19 +37,12 @@ func ConfigureVpnClientFactory(

statsSaver := bytescount.NewSessionStatsSaver(statsKeeper)

consumerCountry, err := locationDetector.DetectCountry()

This comment has been minimized.

Copy link
@Waldz

Waldz May 8, 2018

Member

Why not detecting on connection start?

This comment has been minimized.

Copy link
@interro

interro May 8, 2018

Author Contributor

fixed

This comment has been minimized.

Copy link
@Waldz

Waldz May 10, 2018

Member

Does it mean that we are always getting startup location?

This comment has been minimized.

Copy link
@interro

interro May 10, 2018

Author Contributor

Getting location on app start

@@ -61,14 +61,22 @@ func NewCommandWith(
filepath.Join(options.DirectoryConfig, options.LocationDatabase),
)

originalLocation, err := locationDetector.DetectLocation()

This comment has been minimized.

Copy link
@Waldz

Waldz May 8, 2018

Member

Factory should just construct, not actual jobs

}

response := struct {
Original location.Location `json:"original"`

This comment has been minimized.

Copy link
@Waldz

Waldz May 8, 2018

Member

Internal Location object should be mapped to Tequilapi response DTO

@Waldz
Copy link
Member

left a comment

LGTM. Just have 1 question

@@ -118,6 +123,13 @@ func (cmd *Command) Start() error {
}
log.Infof("Api started on: %d", port)

originalLocation, err := cmd.locationCache.RefreshAndGet()

This comment has been minimized.

Copy link
@Waldz

Waldz May 10, 2018

Member

Why are't You using/injecting variable originalLocation anythere?

This comment has been minimized.

Copy link
@interro

interro May 10, 2018

Author Contributor

Because after calling RefreshAndGet() location is fetched and stored in cache for future uses

This comment has been minimized.

Copy link
@tadovas

tadovas May 10, 2018

Member

By the way - I suggest moving log.xx line after location is fetched. In case of error it should not display Api started... and then error

This comment has been minimized.

Copy link
@interro

interro May 10, 2018

Author Contributor

Done


// GetLocation responds with original and current countries
func (ce *LocationEndpoint) GetLocation(writer http.ResponseWriter, request *http.Request, params httprouter.Params) {
originalLocation, err := ce.locationCache.Get()

This comment has been minimized.

Copy link
@tadovas

tadovas May 10, 2018

Member

If we failed to get location (original) on startup do we always display that error?

This comment has been minimized.

Copy link
@zolia

zolia May 10, 2018

Member

I would make it '(le *LocationEndpoint)', not '(ce *LocationEndpoint)'

This comment has been minimized.

Copy link
@donce

donce May 10, 2018

Contributor

If we failed to get location (original) on startup do we always display that error?

In case only one of original and current ip is not available, it makes sense to return what we have. I.e.

{
  original: null,
  current: "1.1.1.1"
}

or

{
  original: "1.1.1.1",
  current: null
}

This comment has been minimized.

Copy link
@interro

interro May 10, 2018

Author Contributor

@tadovas changed logic where locationCache.Get() no longer returns error so the answer is no, we display error on startup only

This comment has been minimized.

Copy link
@interro

interro May 10, 2018

Author Contributor

@donce Currently if some data is not available, it has empty value

{
 "original": {
  "ip": "", 
  "country": ""
 },
 "current": {
  "ip": "1.1.1.1",
  "country": ""
 }
}
}

response := struct {
Original location.Location `json:"original"`

This comment has been minimized.

Copy link
@donce

donce May 10, 2018

Contributor

original looks confusing - it doesn't specify what this location actually means. Maybe startup would be more clear?

This comment has been minimized.

Copy link
@tadovas

tadovas May 10, 2018

Member

startup exposes 'when' it was looked up, while 'original' represents real collected country. I would go for original

This comment has been minimized.

Copy link
@Waldz

Waldz May 11, 2018

Member

original is just perfect

This comment has been minimized.

Copy link
@donce

donce May 11, 2018

Contributor

ok, when you put it like this, it does make sense - but at first look, it didn't

// Cache allows caching location
type Cache interface {
Get() (Location, error)
RefreshAndGet() (Location, error)

This comment has been minimized.

Copy link
@donce

donce May 10, 2018

Contributor

Most of uses of RefreshAndGet seems to be made only for Refresh part, result is ignored.
We can simplify this method to assigning single responsibility to it - just Refresh().

This comment has been minimized.

Copy link
@Waldz

Waldz May 11, 2018

Member

I agree. And printing can just take with cache.Get()


// GetLocation responds with original and current countries
func (ce *LocationEndpoint) GetLocation(writer http.ResponseWriter, request *http.Request, params httprouter.Params) {
originalLocation, err := ce.locationCache.Get()

This comment has been minimized.

Copy link
@donce

donce May 10, 2018

Contributor

If we failed to get location (original) on startup do we always display that error?

In case only one of original and current ip is not available, it makes sense to return what we have. I.e.

{
  original: null,
  current: "1.1.1.1"
}

or

{
  original: "1.1.1.1",
  current: null
}
@Waldz

Waldz approved these changes May 11, 2018

@donce

donce approved these changes May 11, 2018

if err != nil {
log.Warn("Failed to detect country", err)
} else {
log.Info("Country detected: ", originalLocation.Country)

This comment has been minimized.

Copy link
@donce

donce May 11, 2018

Contributor

Logs of original and vpn country could differ - i.e. "Original country detected: Lithuania", "VPN country detected: China" - it would be easier to debug.

This comment has been minimized.

Copy link
@interro

interro May 11, 2018

Author Contributor

added "original" to log message

}

// Stores location to cache
func (lc *cache) RefreshAndGet() (Location, error) {

This comment has been minimized.

Copy link
@donce

donce May 11, 2018

Contributor

Refresh()?

@interro interro merged commit 2454ffc into master May 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@interro interro deleted the feature/MYST-532-location-endpoint branch May 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.