Conversation
nelsonkopliku
force-pushed
the
basic-ai-config-functions
branch
from
e1467b0 to
e158e52
Compare
| @@ -0,0 +1,49 @@ | |||
| defmodule Trento.AI do | |||
There was a problem hiding this comment.
The flow overall is:
If AI is enabled system wide, it can be enabled on a per user basis; what is not clear to me is how to disable it once enabled at the user level. I wonder if we also need an AI configuration enabled_at (either a ts or nil) or similar field?
There was a problem hiding this comment.
As discussed privately: yes there is a missing piece in the picture, that is "once I have enrolled for AI, how can I opt out?". Meaning that we might need to consider a user action to either:
- clear out its whole AI configuration
- enable/disable it
Will raise it with @abravosuse and @jagabomb
balanza
added
env
perfectly balanced, as always should be |
|
The preview environment for this pull request is ready at 4140.prenv.trento.suse.com. |
| add :user_id, references(:users, on_delete: :delete_all), primary_key: true | ||
| add :provider, :string | ||
| add :model, :string | ||
| add :api_key, :binary |
There was a problem hiding this comment.
issue: As far as I understand, we are storing unencrypted user's api keys in the application database, are we? If so, I think we should rethink this approach.
There was a problem hiding this comment.
We should be covered here by
field :api_key, EncryptedBinary, redact: trueLet me know if you find something I missed.
nelsonkopliku
force-pushed
the
basic-ai-config-functions
branch
from
58daf83 to
e8f9f91
Compare
arbulu89
left a comment
arbulu89
left a comment
There was a problem hiding this comment.
Hey!
I just did a superficial review.
The core logic looks good.
I'm just wondering of the extra complexity of the enabled logic, which protects parts of code that shouldn't be accessible for regular users
lib/trento/ai.ex
Outdated
|
|
||
| See `Trento.AI.Configurations.create_user_configuration/2` for more details. | ||
| """ | ||
| def create_user_configuration(user, attrs) do |
There was a problem hiding this comment.
question:
is this not a bit of an overkill?
no user can access this functions as long as the api routes are disabled.
it would only be available from the console, which i don't find it dangerous.
I don't know, it adds a layer of complexity that might not be needed.
nelsonkopliku
force-pushed
the
basic-ai-config-functions
branch
from
e8f9f91 to
6095286
Compare
gagandeepb
left a comment
gagandeepb
left a comment
There was a problem hiding this comment.
Thanks for this. I would be fine if we can revisit the enable/disable of the AI config in this or in a follow up PR.
nelsonkopliku
force-pushed
the
basic-ai-config-functions
branch
from
6095286 to
ecb3839
Compare
4 participants
Description
This PR introduces the scaffolding to support users' AI onboarding/configuration.
Create/Update functions added.
What's next:
Trying to keep the PRs as contained as possible.
Half of the changes in this PR are tests 🙈