-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix overloading config and missing id in response #35
Fix overloading config and missing id in response #35
Conversation
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! I only have basically 2 questions about design changes. but overall this looks like an improvement. Thank you!
@@ -224,8 +230,7 @@ async def show_execution_ids(): | |||
return [log.stem for log in logs.iterdir() if is_execution_finished(log)] | |||
|
|||
|
|||
def _start_all_robot_tasks(id: str, variables: list = None) -> int: | |||
config = RFS_Config().cmd_args | |||
def _start_all_robot_tasks(id: str, variables: list, config) -> int: |
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 do we make variables
a mandatory argument?
If we make it mandatory, we can remove the following 2 lines, too and demand variables to at least an empty list.
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.
You are right : variables
is always None
. I will remove variables
from _start_all_robot_tasks
.
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.
I have pushed the changes
@@ -243,8 +248,7 @@ def _start_all_robot_tasks(id: str, variables: list = None) -> int: | |||
) | |||
|
|||
|
|||
def _start_specific_robot_suite(id: str, suite: str, variables: list = None) -> int: | |||
config = RFS_Config().cmd_args | |||
def _start_specific_robot_suite(id: str, suite: str, variables: list, config) -> int: |
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.
same as above, marked for later tracability: Why do we make variables a mandatory argument?
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.
This time, variables
is always passed with _start_specific_robot_suite
method. I think it should be mandatory.
Examples :
@router.get("/run/suite/{suite}", tags=["execution"])
async def run_suite(suite, request: Request):
#...
variables = RequestHelper.parse_variables_from_query(request)
#...
response = await run_robot_and_wait(
request.app.state.executor,
func=_start_specific_robot_suite,
args=[id, suite, variables, config],
)
We can remove the following 2 lines :
if variables is None:
variables = []
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.
I have pushed the changes
@@ -263,8 +267,7 @@ def _start_specific_robot_suite(id: str, suite: str, variables: list = None) -> | |||
) | |||
|
|||
|
|||
def _start_specific_robot_task(id: str, task: str, variables: list = None) -> int: | |||
config = RFS_Config().cmd_args | |||
def _start_specific_robot_task(id: str, task: str, variables: list, config) -> int: |
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.
same as above, marked for later tracability: Why do we make variables a mandatory argument?
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.
variables
is always passed with _start_specific_robot_task
method. I think it should be mandatory.
We can remove the following 2 lines :
if variables is None:
variables = []
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.
I have pushed the changes
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! I agree with you on the suggested changes about the variables parameter and the obsolete lines.
d0c87b8
to
c3ee42e
Compare
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.
LGTM
It was impossible to overload the config of the server.
I run the server on my local machines (Windows and MacOs) with options :
In
RobotFrameworkService/routers/robotframework.py
:RFS_Config().cmd_args
is alwaysDefaultConfig
The fix is to move up the call of
config=RFS_Config().cmd_args
in the async router method. I imagine that coroutines like_start_specific_robot_task
are valued before execution and so they get the default value ofRFS_Config
.There is another fix, in a second commit, for returning the id of test run.