-
Notifications
You must be signed in to change notification settings - Fork 137
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
[Design] Introduce GCP Secrets for Gen AI services #1697
[Design] Introduce GCP Secrets for Gen AI services #1697
Conversation
…ite with images ..
…odb, gen-ai, iAdvize)
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.
Some changes to take into account
|
||
### Breaking changes | ||
|
||
* Default value of `tock_gen_ai_orchestrator_secret_storage_prefix_name` currently `/dev` shouldn't use slashes as it's not allow according to GCP Secret Names constraints. It will be changed to `dev` but it might break all running project that doesn't defined it. |
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.
The ‘break’ is not linked to the presence of the slash, as the prefix for GCP is also standardised. However, it is linked to the default value of the prefix, which has been changed to ‘LOCAL/TOCK’.
The following is a list of the changes made to the existing environment variables:
'tock_database_credentials_provider' replaced by 'tock_database_mongodb_secret_manager_provider'
'tock_iadvize_credentials_provider' replaced by 'tock_iadvize_secret_manager_provider'
'aws_iadvize_credentials_secret_id' replaced by 'tock_iadvize_credentials_secret_name'
'tock_gen_ai_orchestrator_secret_storage_type' replaced by 'tock_gen_ai_secret_manager_provider'
'tock_gen_ai_orchestrator_secret_storage_prefix_name' replaced by 'tock_gen_ai_secret_prefix'
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.
Added at the end for @AurelienSylvan to be renamed / updated in helm charts also.
@sacquatella those environment variables will have impact on the Helm Charts they are all specified in this design document that will be available on the EN version of tock website, I'll create on issue on https://github.com/theopenconversationkit/tock-helm-chart so that we don't forget it.
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.
Issue created here : theopenconversationkit/tock-helm-chart#2
Co-authored-by: AurelienSylvan <[email protected]>
Comment from AurelienSylvan theopenconversationkit#1697 (comment)
Co-authored-by: AurelienSylvan <[email protected]>
Co-authored-by: assouktim <[email protected]>
Comment from assouktim theopenconversationkit#1697 (comment)
The enrinomment variables are pretty much everywhere in the doc, it's possible that i've skipped the review of some of them. I suggest that you make a single reference (table) of these env variables, and add clonnes to specify the component that must load them. |
@Benvii I've finished making the changes I wanted, you can check now. |
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.
Ok for me (I can't approve on my own PR).
Fixes #1695