-
Notifications
You must be signed in to change notification settings - Fork 734
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
Remove global nature of parameters and flow decorators #1886
Conversation
This scopes decorators and parameters to the flow itself which should improve compatibility with the runner
Duh -- I think I need to import the cli inside the context in the R code. I'll make the change. |
@@ -391,6 +419,10 @@ def wrapper(cmd): | |||
cmd.has_flow_params = True |
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.
cmd.has_flow_params = True
is used to determine where to dynamically inject user defined parameters...
they are to be injected in commands like run
and sub-commands such as trigger
(for argo-workflows
, etc.)
It is used in the following snippet in click_api.py
:
def extract_command(
cmd_obj: click.Command, flow_parameters: List[Parameter]
) -> Callable:
if getattr(cmd_obj, "has_flow_params", False):
for p in flow_parameters[::-1]:
cmd_obj.params.insert(0, click.Option(("--" + p.name,), **p.kwargs))
the issue present in #1885 is basically the following:
- there's a parent flow with parameters
date
andx
- there's a child flow with parameters
date_child
andx
while the flow_parameters
are date_child
and x
-- this is correct...
the cmd_obj.params
already has date
and x
existing within it at this time -- this needs to be fixed..
Testing[766] @ bcde1b4 |
This scopes decorators and parameters to the flow itself which should improve compatibility with the runner