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

Add API POST /robotframework/run #37

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

remi-picard
Copy link

I suggest to add a new generic API POST endpoint to handle all possible robot CLI options (running test, task, suite... or overloading variables or changing loglevel... eventualy all possibles robot options).

Execution can be synchronous or asynchronous.

For example :

image

Options implemented (Swagger Schema) :

image

Examples in Swagger :

image

Old execution GET API could be deprecated :
image

@JianZhou12345678
Copy link

Wow, that's great. It's exactly what I need. Thank you very much

Copy link
Member

@Noordsestern Noordsestern left a comment

Choose a reason for hiding this comment

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

Great idea! I rather not deprecate the GET endpoints, though. Other than that, I think, these changes should be merged and improve the webservice. Thank you 🥇

@@ -54,7 +54,7 @@ async def run_robot_and_wait(executor: Executor, func, args=[], kwargs={}):
)


@router.get("/run/all", tags=["execution"])
@router.get("/run/all", tags=["execution"], deprecated=True)
Copy link
Member

Choose a reason for hiding this comment

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

I would not like to deprecate the GET endpoint. With POST we always need a body which requires a bit logistics on the client side. GET is simply calling an URL.

I know it is only convenience and debatable if GET is meant for triggering an action. But it was very useful for me when hosting robot-werservice in Google Cloud Run.

I would like to keep GET and add the POST endpoint

Copy link
Author

Choose a reason for hiding this comment

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

I have just removed that.

@Noordsestern Noordsestern self-assigned this Feb 18, 2024
@remi-picard remi-picard force-pushed the api-run branch 2 times, most recently from 8c151ba to e700544 Compare February 18, 2024 12:40
@remi-picard
Copy link
Author

Great idea! I rather not deprecate the GET endpoints, though. Other than that, I think, these changes should be merged and improve the webservice. Thank you 🥇

Ok, no problem, I remove the deprecation changes.

import asyncio
import multiprocessing as mp
from concurrent.futures import Executor
from typing import Annotated
Copy link
Member

Choose a reason for hiding this comment

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

The import of Annotated seems to break with Python < 3.10

I am fine with dropping support for Python 3.7, since it is end-of-lie. I am not so sure though about dropping support for 3.8 and 3.9.

Can we make this somehow compatible with 3.8?

Copy link
Author

Choose a reason for hiding this comment

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

You are right, there are some incompatibilities with Python < 3.10.

For example, I have to change import typing to typing_extensions (https://fastapi.tiangolo.com/python-types/#__tabbed_8_2)

I will try to fix 3.8 and 3.9.

Copy link
Author

Choose a reason for hiding this comment

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

It is OK, I also change | with Optional (https://fastapi.tiangolo.com/python-types/#using-union-or-optional)

FastAPI is really well documentated !

@remi-picard remi-picard force-pushed the api-run branch 5 times, most recently from 294f215 to 244461b Compare February 18, 2024 13:43
@Noordsestern Noordsestern merged commit c5c5b9d into MarketSquare:master Feb 18, 2024
6 checks passed
@Noordsestern Noordsestern mentioned this pull request Feb 19, 2024
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