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

feat: add controller to as-controller-board-status #43

Merged

Conversation

vli11
Copy link
Contributor

@vli11 vli11 commented Sep 21, 2022

closes: issue #40
Signed-off-by: Valina Li valina.li@intel.com

closes: issue intel-retail#40
Signed-off-by: Valina Li <valina.li@intel.com>

func (c *Controller) AddAllRoutes() error {
// Add the "status" REST API route
err := c.service.AddRoute("/status", c.GetStatus, "GET", "OPTIONS")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http.MethodGet, http.MethodOptions for constants

@@ -6,7 +6,7 @@ package functions
import (
"time"

"github.com/edgexfoundry/app-functions-sdk-go/v2/pkg/interfaces"
clientInterfaces "github.com/edgexfoundry/go-mod-core-contracts/v2/clients/interfaces"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

go convention is to only alias when you have a name conflict, which isn't the case here

as-controller-board-status/main.go Show resolved Hide resolved
SubscriptionClient: subscriptionClient,
}

// Subscribe to the EdgeX notification service
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove command since this is obvious from the code.

Signed-off-by: Valina Li <valina.li@intel.com>
Copy link
Contributor

@jim-wang-intel jim-wang-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please resolve my concerns thanks

Comment on lines 15 to 20
// RESTPost is a const used for REST commands using the specified method.
RESTPost = "POST"
RESTPost = http.MethodPost
// RESTPut is a const used for REST commands using the specified method.
RESTPut = "PUT"
RESTPut = http.MethodPut
// RESTGet is a const used for REST commands using the specified method.
RESTGet = "GET"
RESTGet = http.MethodGet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remove these constants and use the ones from http. in place of them

Copy link
Contributor

@jim-wang-intel jim-wang-intel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all my issues addressed - thanks

Signed-off-by: Valina Li <valina.li@intel.com>
@vli11 vli11 self-assigned this Sep 21, 2022
utilities.WriteJSONHTTPResponse(writer, req, http.StatusOK, controllerBoardStatusJSON, false)
})
// for testing purpose
func (boardStatus *CheckBoardStatus) SetControllerBoardStatus(controllerBoardStatusInput ControllerBoardStatus) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually Set is not needed for testing since as @jim-wang-intel pointed out the tests have direct access to controllerBoardStatusInput . This can be cleaned up in the next PR.

Copy link
Contributor

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vli11 vli11 merged commit 97cbcf5 into intel-retail:EdgeX-2.2 Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants