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

Support getting the PSU state with a GET request #141

Merged
merged 1 commit into from
Dec 17, 2019

Conversation

nicksinger
Copy link
Contributor

What does this PR do and why is it necessary?

According to common practices GET is used to retrieve resource representation/information only. This PR enables the plugin to respond to get requests to its API-endpoint with the current PSU state.

How was it tested? How can it be tested by the reviewer?

I hot-patched the installed plugin on my octoprint instance with the newly added function. Afterwards I used curl to fetch the new endpoint:

$ curl -s -H "X-Api-Key: <my-api-key>" http://octoprint.example.com/api/plugin/psucontrol
{            
  "isPSUOn": false
}
$ curl -s -H "Content-Type: application/json" -H "X-Api-Key: <my-api-key> -X POST -d '{ "command":"turnPSUOn" }' http://octoprint.example.com/api/plugin/psucontrol
$ curl -s -H "X-Api-Key: <my-api-key>" http://octoprint.example.com/api/plugin/psucontrol
{            
  "isPSUOn": true
}

For reviewers I suggest to do the same or alternatively point pip to install from my branch.

Any background context you want to provide?

My main motivation was to get the PSU control available in my home-assistant instance. For this I wanted to use the RESTful Switch component. This component is implemented as such that it retrieves the status over GET and sends updates over POST (following the best-practices linked above). To quote: "The switch can get the state via GET and set the state via POST on a given REST resource."

Further notes

I implemented this feature by calling the original implementation of this. This allows backwards compatibility and makes sure the same access restrictions are in place. Technically speaking I think that the "getPSUState"-POST-request should be removed but this is not a decision I wanted to take (especially since it might break existing setups).

@kantlivelong
Copy link
Owner

Thanks for the PR.

Definitely will have to keep getPSUState for POST (no harm anyway) but adding GET totally makes sense. I also don't see any harm in sending getPSUState for unauthorized users so I may adjust that.

@kantlivelong kantlivelong merged commit f539821 into kantlivelong:devel Dec 17, 2019
@easytarget
Copy link

A quickie note to assure you that keeping the POST request method is good, I used it last year here to sense the power state of the printer being monitored.

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.

3 participants