Settings sync follow-up: Convert setting_type column from text to enum
The following discussion from !134571 (merged) should be addressed:
-
@Alexand started a discussion: (+3 comments) suggestion / question
I'm sorry, @ealcantara.
🤦 But I only realized this now, and I think it's relevant to the current change. We could think about this also in a follow-up, but it might mean more reworking in the future.Have we considered having the
setting_type
to be anenum
? It feels like it's the perfect case for anenum
as the values are predefined by the upstream VS Code API specification and the users don't determine the text that goes there, right?If this is the case, I think we should consider making this attribute an
enum
in Rails, and asmallint
in the database table. See https://docs.gitlab.com/ee/development/database/creating_enums.html.The
smallint
will make the unique index smaller, the table data used smaller, and we don't need this check constraint anymore:CONSTRAINT check_994c503fc4 CHECK ((char_length(setting_type) <= 256))
.Although, this would mean that you'd need to do some more changes to your code here. Do we have a good reason to not have an enum, but a text column instead?
We can also push this question to the database maintainer to weigh-in if you prefer.
In practice this change would mean:
- Changing the column type.
- Creating an enum out of
SETTINGS_TYPES
, which goes in line with that idea of bringing it to the model instead of theservicelibrary. - So there might be a small refactor to be done on the code where you currently call
SETTINGS_TYPES
too, so to reference the new enum keys instead of the constanthasharray. You can still just move that constant to the model and assign the enum like:enum setting_type: SETTING_TYPES
, but it needs to become a hash with numbers as values. Then I think you can callSETTING_TYPES.keys
if you want to get an array with value names.