Support for notifications in Mistral #1516
Conversation
… string and not a dict result. Note: This only happens in some edge rare edge cases.
…h is disabled and continue with the request. If the auth is indeed enabled, but auth API is unavailable, request to the API will fail with invalid credentials error.
Remove mistral-v1 runner from the available runner script and remove all the references to it.
| except requests.exceptions.ConnectionError as e: | ||
| LOG.warn('API server is not available, skipping authentication.') | ||
| LOG.exception(e) | ||
| return client |
There was a problem hiding this comment.
Is this intentional? Why does this return a client?
EDIT: This is the case when auth is required cos user supplied username and password. If Auth API server is not available, we should bubble the exception all the way up?
There was a problem hiding this comment.
Yes, we still want to continue.
I was thinking about it, but bubbling the exception up instead of continuing makes it harder to test and switch between servers which have auth enabled and disabled.
There was a problem hiding this comment.
I do see it. As a client, we have no idea if server is auth enabled or not. It looks like there needs to a handshake at the start to discover server capabilities. I am not proposing we do it now but it looks like we might soon need it.
There was a problem hiding this comment.
That said, I am curious how would client.* operations would work if auth server died but auth is enabled in server. We'd keep getting 401s back?
There was a problem hiding this comment.
It doesn't matter as long as you have a valid token. If the auth is down, but you have a valid token, all the operations will still work until the token expires.
|
LGTM |
Support for notifications in Mistral
This pull request adds support for notifications in Mistral workflows. It works in exactly the same way it works in the action runner.
I also noticed
mistral-v1runner is still registered even though we removed the Python module quite a while ago. I removed the runnersregistrar entry and all the references to it.Note: This PR is based on #1514.
Related PRs