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-268 Add endpoint and CLI command for current IP #118

Merged
merged 9 commits into from Jan 26, 2018

Conversation

Projects
None yet
3 participants
@donce
Copy link
Contributor

commented Jan 24, 2018

No description provided.

@donce donce requested review from ignasbernotas, tadovas and Waldz Jan 24, 2018

@@ -110,6 +110,20 @@ func (client *Client) Status() (status StatusDto, err error) {
return status, err
}

func (client *Client) Ip() (string, error) {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

GetIP()

defer response.Body.Close()

var ipData struct {
Ip string `json:"ip"`

This comment has been minimized.

Copy link
@Waldz
)

type currentIPEndpoint struct {
ipResolver ipResolver

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 24, 2018

Member

All ipifyClient could be this dependency. And finally it could be renamed ipifyClient -> IPResolver

This comment has been minimized.

Copy link
@donce

donce Jan 25, 2018

Author Contributor

Not quite sure I understood the suggestion - are you suggesting to refactor ipify.Client interface with all its implementations into functions (instead of a classes), i.e.:
func ipify.NewClient (ipify.Client, error) -> func ipify.NewIPResolver (IPResolver, error) ?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

No, I suggest to inject full ipifyClient here ipResolver ipify.Client

@Waldz

Waldz approved these changes Jan 25, 2018

func NewAPIRouter() *httprouter.Router {
router := httprouter.New()
router.HandleMethodNotAllowed = true

router.GET("/healthcheck", endpoints.HealthCheckEndpointFactory(time.Now, os.Getpid).HealthCheck)

ipifyClient := ipify.NewClient()
router.GET("/ip", endpoints.NewCurrentIPEndpoint(ipifyClient.GetPublicIP).CurrentIP)

This comment has been minimized.

Copy link
@tadovas

tadovas Jan 25, 2018

Member

Maybe we can make a more generic endpoint and call it /network which can return more network information, not only ip?

This comment has been minimized.

Copy link
@donce

donce Jan 25, 2018

Author Contributor

It's not hard to rename later on if we have to add more fields - for now, all it does is return IP, because that's the user story.

func NewAPIRouter() *httprouter.Router {
router := httprouter.New()
router.HandleMethodNotAllowed = true

router.GET("/healthcheck", endpoints.HealthCheckEndpointFactory(time.Now, os.Getpid).HealthCheck)

ipifyClient := ipify.NewClient()
router.GET("/ip", endpoints.NewCurrentIPEndpoint(ipifyClient.GetPublicIP).CurrentIP)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

It should go somewhere under /v1..

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

Maybe GET /v1/connection/ip is right place

This comment has been minimized.

Copy link
@donce

donce Jan 25, 2018

Author Contributor

We don't have /v1, but it makes sense to move it inside connection.

This comment has been minimized.

Copy link
@donce

donce Jan 25, 2018

Author Contributor

moved to connection

func NewAPIRouter() *httprouter.Router {
router := httprouter.New()
router.HandleMethodNotAllowed = true

router.GET("/healthcheck", endpoints.HealthCheckEndpointFactory(time.Now, os.Getpid).HealthCheck)

ipifyClient := ipify.NewClient()

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

Dont factory on the fly, pass dependency from upper layer

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

But I just noticed that NewAPIRouter() factory is not right place to connect features

This comment has been minimized.

Copy link
@donce

donce Jan 25, 2018

Author Contributor

Moved to connection endpoints

This comment has been minimized.

Copy link
@donce

donce Jan 25, 2018

Author Contributor

Also, added dependency injection.

@donce donce force-pushed the feature/MYST-268-current-ip-endpoint branch from 476781d to 82545d1 Jan 25, 2018

@donce

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

@Waldz fixed all issues, ready for review :)

const IPIFY_API_CLIENT = "goclient-v0.1"
const IPIFY_API_LOG_PREFIX = "[ipify.api] "
const IpifyAPIURL = "https://api.ipify.org/"
const IpifiAPICLient = "goclient-v0.1"

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

Ipifi -> Ipify..

  • Could be private constants

This comment has been minimized.

Copy link
@donce

donce Jan 25, 2018

Author Contributor

Done.


type Client interface {
type Resolver interface {
GetPublicIP() (string, error)

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 25, 2018

Member

Could be 2 implementations with same ip. Resolver interface e.g. NewPublicResolver(), NewOutboundResolver()

@donce donce force-pushed the feature/MYST-268-current-ip-endpoint branch from 8fc707d to 738377e Jan 25, 2018

@donce donce force-pushed the feature/MYST-268-current-ip-endpoint branch from 738377e to 1552e06 Jan 25, 2018

@Waldz

Waldz approved these changes Jan 25, 2018

@donce donce merged commit 49a08fc into master Jan 26, 2018

@donce donce deleted the feature/MYST-268-current-ip-endpoint branch Jan 26, 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.