-
Notifications
You must be signed in to change notification settings - Fork 74
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
Adds PostgreSQL Database Table Schema to Configuration #677
base: main
Are you sure you want to change the base?
Conversation
Hi @chriscannon. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/kind feature |
/ok-to-test |
The following is the coverage report on the affected files.
|
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.
/approve
Looks good to me!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adambkaplan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@adambkaplan could you also add a lgtm label so that it can pass the CI validation? Thanks! |
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.
I gave this a bit closer scrutiny before providing lgtm.
@chriscannon are you able to squash your commits and provide a descriptive commit message once the items below are addressed?
@tektoncd/operator-maintainers fyi I think this feature would impact any operator-managed deployment of Tekton Results
cmd/api/main.go
Outdated
@@ -100,7 +98,9 @@ func main() { | |||
var err error | |||
|
|||
dbURI := fmt.Sprintf("host=%s user=%s password=%s dbname=%s port=%s sslmode=%s sslrootcert=%s", serverConfig.DB_HOST, serverConfig.DB_USER, serverConfig.DB_PASSWORD, serverConfig.DB_NAME, serverConfig.DB_PORT, serverConfig.DB_SSLMODE, serverConfig.DB_SSLROOTCERT) | |||
gormConfig := &gorm.Config{} | |||
gormConfig := &gorm.Config{NamingStrategy: schema.NamingStrategy{ |
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.
It is not readily clear from the code that this is how you set the Postgresql "schema" using gorm. Can you please add a comment with a link to the relevant docs in gorm?
@@ -3,6 +3,7 @@ DB_PASSWORD= | |||
DB_HOST= | |||
DB_PORT=5432 | |||
DB_NAME= | |||
DB_SCHEMA=public |
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.
This becomes the default "table prefix" value in our distribution. Will this break the database initialization/auto-migration phase of the deployment on upgrade? Is there risk of data being "orphaned" because the apiserver will create a whole new schema in Postgres?
@chriscannon: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Changes
This allows a schema to be set on the table instead of assuming
public
.Submitter Checklist
These are the criteria that every PR should meet, please check them off as you review them:
/kind <type>
. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes