-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Don't raise dependency errors for dynamic credential fields #4798
Don't raise dependency errors for dynamic credential fields #4798
Conversation
Example: setting ssh_key_unlock by hand when the key comes from a vault.
Build succeeded.
|
For #4791 |
So I guess technically this pull request DOES solve a bug, and should be merged. But it uncovers a deeper underlying bug too. |
Anything in the logs that looks like a traceback or gives more info as to how it failed? |
Actually you know what, I forgot to make this code change on the task container. I'll work that out and get back to you with any additional logging. |
@@ -652,6 +652,9 @@ def validate(self, value, model_instance): | |||
) | |||
if match: | |||
label, extraneous = match.groups() | |||
# bail if the dependency is for a dynamic source |
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.
For new credentials, these dynamic input relationships don't exist yet, and the dependency validation error will still occur:
To get the expected validation behavior, we'll need to come up with a way of knowing the intended dynamic inputs before they've been created.
For regular / static fields that are encrypted, we've dealt with similar issues in the past by (ab)using the special $encrypted
value as a placeholder. We may need to do something similar here.
I'd need to take a deeper look at this before proposing anything specific.
@omgjlk Once the prompted fields are saved, I suspect that an updated task container might be all that's needed to resolve #4798 (comment) but it would be good to confirm - thanks for taking a look. |
I restarted the task container, which seems to have already had the edited code in it (I'm guessing with the docker-compose style of deployment there is a shared venv). Unfortunately the task still fails, and there aren't really any good logs to show WHY it failed.
The web UI shows this for output
I'm totally wiling to drop a |
@wenottingham @jakemcdermott @omgjlk I'm not certain this is going to work, and the more I think about it, I think supporting this new dynamic input model means we're going to have to lose this level of validation, because the order of operations just doesn't work. |
@wenottingham we might end up going with this instead: |
Example: setting ssh_key_unlock by hand when the key comes from a vault.