-
Notifications
You must be signed in to change notification settings - Fork 83
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
feat: add ping v2 api #387
Conversation
Signed-off-by: Mike <[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, just a couple naming comments
@@ -29,6 +30,7 @@ import ( | |||
"github.com/edgexfoundry/app-functions-sdk-go/internal/telemetry" | |||
"github.com/edgexfoundry/go-mod-core-contracts/clients" | |||
"github.com/edgexfoundry/go-mod-core-contracts/clients/logger" | |||
gmcccommon "github.com/edgexfoundry/go-mod-core-contracts/v2/dtos/common" |
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.
How about contracts
for the alias?
@@ -83,6 +86,25 @@ func TestConfigureAndPingRoute(t *testing.T) { | |||
body := rr.Body.String() | |||
assert.Equal(t, "pong", body) | |||
|
|||
} | |||
func TestConfigureAndPingV2Route(t *testing.T) { |
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 Configure
in the name?
Co-authored-by: Walt <[email protected]> Signed-off-by: Mike <[email protected]>
FYI, I think we need to revisit the PING request since this is used by Consul, which will want to do a GET w/o a DTO... I will have Jim White put it on the Core WG agenda. |
Exactly why I have a problem with POST. Consul can do a post as well, but the complexity of this API call went from 0 to 100. The consequence of this - can no longer pop the URL in the browser to see if this service is up, the complexity increase for simple curl commands now includes a payload making HEALTHCHECKs for docker and other platforms (i.e. consul) unnecessarily verbose. |
Signed-off-by: Mike [email protected]
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number:
What is the new behavior?
Does this PR introduce a breaking change?
Are there any new imports or modules? If so, what are they used for and why?
Are there any specific instructions or things that should be known prior to reviewing?
Other information