Add support for test credentials in tools #21643
Conversation
arash77
changed the title
arash77
force-pushed
the
add-credentials-tests
branch
from
0c895a5 to
c388cf5
Compare
… lines" This reverts commit 48892b8.
arash77
force-pushed
the
add-credentials-tests
branch
from
5bb4368 to
eef2ffa
Compare
arash77
changed the title
|
|
||
| # Handle credentials_context - can be either: | ||
| # - JSON string (legacy API from planemo) | ||
| # - dict (test mode with embedded values from planemo) |
There was a problem hiding this comment.
Tool tests can be run different ways than Planemo - I don't really think any of these comments should mention Planemo.
Can we rename TestCredentialValue -> DirectCredentialValue and and CredentialsContext -> VaultCredentialsContext. I think if we didn't explicitly have so much test code in the backend runtime I would feel better.
If Planemo cannot do this today - why are there two different ways the credentials might come in (JSON string vs dict)? That doesn't make sense to me - this is all new functionality right - can we just have a normal run parameter that is the directly applying the credentials and just document for most production applications it shouldn't be used and the Vault should be used instead?
| **extra_data, | ||
| ) | ||
| if credentials_context: | ||
| data["credentials_context"] = dumps(credentials_context) |
There was a problem hiding this comment.
I guess because you're supporting credentials down both of these paths - this is why you have the dispatch down below. I think the Planemo comment confused me.
I guess we need the dumps here right? Because we don't have json=True like below?
| credentials_context: Optional[CredentialsContext | TestCredentialsContext] = None | ||
| if credentials_context_raw: | ||
| # Parse JSON string if needed | ||
| if isinstance(credentials_context_raw, str): |
There was a problem hiding this comment.
I think maybe I would like to see this a bit different - I think keep credentials_context as it is and introduce a "direct_credentials" parameter. On the client side I thought it was a little confusing that it was called credentials_context and not "credentials" or something like that - maybe "direct_credentials" would make sense.
| } | ||
| for service in credentials_context.root | ||
| } | ||
| job.add_parameter("__test_credentials__", json.dumps(test_creds_dict)) |
There was a problem hiding this comment.
Again I would prefer direct credentials - or something else without test in the name. I also think we should create a database field for this. I know that is a lot more work but I can point you at some documentation.
2 participants
Add test credential support for tool definitions (#19084).
Introduces definitions for test credentials and implements parsers to inject them during tool test execution.
How to test the changes?
(Select all options that apply)
License