Feature/add simulator api skeleton #2274
Conversation
Esco441-91
force-pushed
the
feature/add-simulator-api-skeleton
branch
from
6f117fd
to
8acc3ad
Compare
|
Seems you have some pylint error, please fix. The easiest way is to have a local development (see the README), which will run pylon locally and even fix the majority of problems automatically. |
|
Will fix the CI errors. |
Esco441-91
force-pushed
the
feature/add-simulator-api-skeleton
branch
2 times, most recently
from
a55b5f5
to
b18d97f
Compare
|
Fixed. Please let me know if there is anything else. |
There was a problem hiding this comment.
Please do not ping me, and say you changed all, when I see there are still quite a number of open comments. I do have quite a lot to do, so I will not review the same code X times, just to see its unchanged.
Once you have addressed all comments, then click the "ready for review" button.
Esco441-91
force-pushed
the
feature/add-simulator-api-skeleton
branch
from
b18d97f
to
14f0414
Compare
I am very confused over what you meant. I didn't ping that the requested changes are ready, the only thing I did yesterday evening was the comments/clarification requests on the opened discussions. Was it perhaps the prior comment related to CI changes that came in your e-mail post-fact?
This one was before there were any change requests posted. Please let me know. EDIT: Need to fix CI stuff again. |
Esco441-91
force-pushed
the
feature/add-simulator-api-skeleton
branch
from
14f0414
to
b966a16
Compare
|
The CI checks now pass and I've changed the requested. Please let me know if anything else requires amending. |
Actually it was NOT, I read your comment and started a review, until I realized that the majority of the items was not solved. |
I am still very confused. So for you, the post in the picture came AFTER you had reviewed my changes for the first time? If so, then I blame github for something going weird. Regardless, I apologize if the post mentioned caused any confusion. I had no intention on pinging anyone without any changes done. |
No the post came AFTER my first review (17hours ago, and you responded to that with several comments), and BEFORE my second review (15hours ago, same time as your comment)., I just did the THIRD review. Anyhow its history. |
Esco441-91
force-pushed
the
feature/add-simulator-api-skeleton
branch
2 times, most recently
from
3cc6cf6
to
b91af36
Compare
Esco441-91
force-pushed
the
feature/add-simulator-api-skeleton
branch
from
b91af36
to
674c388
Compare
Esco441-91
force-pushed
the
feature/add-simulator-api-skeleton
branch
from
674c388
to
2b4c12c
Compare
|
Apologies for the long silence. Other work stuff got in the way, but it's a poor excuse. What do you feel like about this PR, do you still want it in? I get that long drawn-out PRs are not very attractive to be considered merging. I can live with it. For the record, I suggest checking out |
pytest-asyncio 0.23.8 added a warning that the default event loop scope configuration will be changed in the future, showing many error messages of type: ``` /home/runner/work/pymodbus/pymodbus/venv/lib/python3.9/site-packages/pytest_asyncio/plugin.py:208: PytestDeprecationWarning: The configuration option "asyncio_default_fixture_loop_scope" is unset. The event loop scope for asynchronous fixtures will default to the fixture caching scope. Future versions of pytest-asyncio will default the loop scope for asynchronous fixtures to function scope. Set the default fixture loop scopeexplicitly in order to avoid unexpected behavior in the future. Valid fixture loop scopes are: "function", "class", "module", "package", "session" ``` This commit fixes the warning by doing what it says: it sets the default event_loop scope. More info in [1]. [1]: https://github.com/pytest-dev/pytest-asyncio/blob/main/docs/reference/configuration.rst Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
The endpoints are meant to serve the restapi requests. Change it explicitly to represent that with `restapi/<endpoint>`. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
To make the logic a bit more clear, change the json endpoint names to match the ones with html. This way, the html equivalent and restapi ones are more closely related (`api/registers` or `restapi/registers`). Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
The base was trying to fetch builders from html builders, which is not the intention. Use json generators instead. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
The restapi will not use the generated dict anywhere at the moment, so remove it as unneeded. To match the type hints, add a type hint for callable. Note that it is arguable if the entire table should be moved to the handle_json function, but for now it is left as is. The table represents the default paths that the system follows, and could be integral in understanding the flow of requests.
Prior commit removed the dict from the generator dict. This commit removes the dict usage from the endpoints, including the endpoint functions. The future implementations will not use said information as a parameter, as there is nothing to fill in to it. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
In order to use the simulator json endpoints, use the aiohttp built-in json mechanisms to recieve and send json data. This way we can simply use the json parameters and pass them along to the builders, similar to how they are done in the html endpoints. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
In order to not confuse clients with clear-text error messages, change the simulator to report unhandled (known) errors as json. This will allow the client to handle the error in a more structured way, as well as keep the response content header correct. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
Rename `page_type` to `endpoint` in `handle_json` method. This will make the context a bit clearer to understand. The `endpoint` is used to choose which method to call from the generators dict. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
In order to be able to handle the submit actions with similar behavior in json as in html, the submit handling was changed to be html specific. This meant changing the helper to take the Submit dict as a paramter, and then passing that to the submit helper. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
As a part of the ReST API, add endpoint for handling registers. It is almost a direct copy-paste from the html variant, as to not inroduce differing behavior between the two. The only exception is that the Submit actions use a different set of actions, as some of them do not make sense for json. The submit dict is endpoint specific here to not cause confusion later on the rest of the endpoints. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
Same as with the registers endpoint, this one is more or less a copypaste of the calls html endpoint. Add own submit dict for calls to handle only calls related actions. The calls_json endpoint handles only the simulation part of the calls. Monitoring is not supported. The assumption is that the monitoring is not usable built-in, and without something like WebSocket, the monitoring would still require an API call. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
Log and Server endpoints are not implemented on the simulator, neither in html or json. Make them respond with an error message of them being unimplemented. To be explicit about the future implementations needing to use the `parameters` field, pass it on in response in both of the endpoints. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
As the first step to add a simulator ReST API, this commit adds a base for future tests. The base uses fixtures to setup the simulator and client in a standard way that supports the current simulator design. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
In preparation for the json registers endpoint, add tests for them in advance. This will set the structure of how the implementation will be done. This has been completed mostly by checking what the current HTML builder does, and then replicating the parameters to match JSON requests and responses. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
This commit adds tests for the /api/calls_json endpoint. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
The endpoints "Server" and "log" have not been fully implemented in the simulator api. Neither in the html or json API. To account for some of the endpoints in the future, add a placeholder for the calls to make sure they are filled in once the implementation is finished. Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
Add documentation skeleton for the simulator API skeleton. The documentation provides information about the existing endpoints, their parameters and the expected responses (as examples). Signed-off-by: Esa Laakso <esa.laakso@fidelix.com>
Esco441-91
force-pushed
the
feature/add-simulator-api-skeleton
branch
from
2b4c12c
to
9fa8808
Compare
|
I will review when you mark it as ready to be reviewed ( meaning earlier review comments have been resolved). Long running PRs are not a problem, I have a couple of those, |
|
I've now fixed the requested parts and put the PR up-to-date with the dev branch. Please let me know if you disagree or if there is anything else. |
I think you should close the old changes requested if they are already solved. Then the status of the PR will become pending to merge. I see you have left one discussion opened when you answered "Changed". After this I think you will be able to mark this PR as pending to resolve. Thanks for your contributions :) |
Thanks for the tips. I use mostly devops and gitlab internally, so I'm not that aware on all of the tricks in github :) However, I do not see a way I can mark the change request as "closed" myself. The best I can do is resolve each comment (tried), but this does not close the change request itself. I think @janiversen needs to close them...? |
|
The review comments are closed by hitting "resolved" |
The comments themselves I take it, but not the change request. Or then I'm misunderstanding something. If so, then sorry for the noise. For me it shows everything is resolved, but it still shows "Changes requested" (and "Merging is blocked"). Same as mentioned in https://stackoverflow.com/questions/49780667/github-changes-requested-label-stay-after-i-answered-all-the-code-review-questi . |
Summary
In order to progress the use of simulator, implement the first revision / skeleton of the simulator ReST API. This should be usable for minimal usage of running a simulator and manipulating registers or causing modbus error responses.
This PR is made with the intention that the added API does not change the simulator, and instead tries to work on the existing base as much as possible. This is done so as not to push too many changes, making this PR intrusive or have unintended changes.
Add tests and documentation for later use and hopefully provide more detailed context to this PR.
Just to clarify, this PR in itself might not be release worthy in any ways. It is usable, but the choices made here are most likely not suitable to all use-cases. The API should be set in stone (possibly during this PR) prior to being advertised as complete.
Relates to issue #1284.
Testing