Follow-up from "Make sure newly created tables do not have `gitlab_main` schema set"
The following discussions from !135859 (merged) should be addressed:
-
@DylanGriffith started a discussion: (+1 comment) Nitpick (non-blocking): I feel like this naming convention is a little surprising to see it return an array. It makes sense of being used in the context of building a hash but on it's own it's a little strange. I'd possibly consider naming it
name_and_schema
or possibly just leave it up to the caller to construct this array themselves even though we'd duplicate that a couple of times. -
@DylanGriffith started a discussion: (+2 comments) @manojmj I like this approach. I had only minor feedback and I asked in internal Slack to see if anyone in the DB team has opinions since they've been working on
db/docs
the most. I might also like another review from someone that has worked ondb/docs
to just make sure we're not missing anything here. -
@krasio started a discussion: (+1 comment) non-blocking: To me, the dictionary is all the information under
db/docs/
, while this class here represents a single entry, so maybe a name likeDictionaryEntry
, orDictionary::Entry
will be a better fit?🤔 -
@krasio started a discussion: non_blocking: We can also write this as
Dictionary.new(file_path).tap { |d| d.validate! }
or even have a class method like
Dictionary.validate!(file_path)
that either returns an instance, or raises an error. -
@krasio started a discussion: non-blocking: We can replace these two methods with just one, doing single iteration, instead of going through the collection 3 times:
def tables_having_gitlab_main_schema(starting_from_milestone:) ::Gitlab::Database::GitlabSchema.build_dictionary('').filter_map do |entry| entry.table_name if entry.schema?('gitlab_main') && entry.milestone.to_f >= starting_from_milestone end end
-
@krasio started a discussion: We also have the optional
schema_inconsistencies
, which was just introduced with !134948 (diffs). It's not used yet (other than keeping track), we can add support for it here later on.