[MM-33077] Add migrations for OAuthApps schema #17209
Conversation
mm-cloud-bot
added
the
release-note
mgdelacroix
force-pushed
the
mm-33077-add-migrations-for-oauthapps
branch
from
ce3aa9f
to
19ccc86
Compare
…m-33077-add-migrations-for-oauthapps
| ALTER TABLE oauthapps ADD COLUMN IF NOT EXISTS istrusted boolean DEFAULT false; | ||
| ALTER TABLE oauthapps ADD COLUMN IF NOT EXISTS iconurl VARCHAR(512) DEFAULT ''; |
There was a problem hiding this comment.
In the current system we don't set defaults on creation and usually we prefer to avoid them. I am wondering if we should change these as well.
| CREATE TABLE IF NOT EXISTS oauthapps ( | ||
| id VARCHAR(26) PRIMARY KEY, | ||
| creatorid VARCHAR(26), | ||
| createat bigint, | ||
| updateat bigint, | ||
| clientsecret VARCHAR(128), | ||
| name VARCHAR(64), | ||
| description VARCHAR(512), | ||
| callbackurls VARCHAR(1024), | ||
| homepage VARCHAR(256) | ||
| ); |
There was a problem hiding this comment.
Another general question: I am wondering if it would be simpler for the table creation code to have the most recent version of the table. I am thinking it would be the same logic wise.
There was a problem hiding this comment.
But in that case we would lose the history right? And if we are going to include the down scripts with the reverse steps it wouldn't be consistent. cc @mgdelacroix
…m-33077-add-migrations-for-oauthapps
| sqlStore.CreateColumnIfNotExists("OAuthApps", "IsTrusted", "tinyint(1)", "boolean", "0") | ||
| sqlStore.CreateColumnIfNotExists("OAuthApps", "IconURL", "varchar(512)", "varchar(512)", "") | ||
|
|
||
| sqlStore.CreateColumnIfNotExists("OutgoingWebhooks", "TriggerWhen", "tinyint", "integer", "0") |
There was a problem hiding this comment.
I literally have no idea, It seems like added before as well:
https://github.com/mattermost/mattermost-server/blob/4739e93c16a0d0d884143d5f12c5ee710f6c08cb/db/migrations/mysql/000014_create_outgoing_webhooks.up.sql#L39
With the wrong type?
| as.CreateIndexIfNotExists("idx_oauthaccessdata_user_id", "OAuthAccessData", "UserId") | ||
| as.CreateIndexIfNotExists("idx_oauthaccessdata_refresh_token", "OAuthAccessData", "RefreshToken") |
There was a problem hiding this comment.
We are removing stuff for OAuthAccessData but the PR merely focuses on OAuthApps.
There was a problem hiding this comment.
These are forgotten
- https://github.com/mattermost/mattermost-server/blob/6ac9cdc98767dc7adaa7dc8b2792c9d2c77f5c0f/db/migrations/mysql/000025_create_oauth_access_data.up.sql#L49
- https://github.com/mattermost/mattermost-server/blob/6ac9cdc98767dc7adaa7dc8b2792c9d2c77f5c0f/db/migrations/mysql/000025_create_oauth_access_data.up.sql#L139
| @@ -259,7 +259,6 @@ func New(settings model.SqlSettings, metrics einterfaces.MetricsInterface) *SqlS | |||
|
|
|||
| store.stores.channel.(*SqlChannelStore).createIndexesIfNotExists() | |||
| store.stores.retentionPolicy.(*SqlRetentionPolicyStore).createIndexesIfNotExists() | |||
| store.stores.oauth.(*SqlOAuthStore).createIndexesIfNotExists() | |||
There was a problem hiding this comment.
This also affects other tables that are not covered in here.
amyblais
added
Changelog/Not Needed
Summary
implements: https://mattermost.atlassian.net/browse/MM-33077
Ticket Link
https://mattermost.atlassian.net/browse/MM-33077
Release Note