Consider stubbing Gitlab.com? in the migration context
Often when we trigger db:gitlabcom-database-testing
it does not work well with migrations that have return unless Gitlab.com? || Gitlab.staging?
I think we might want to stub Gitlab.com?
with true
to avoid that and actually perform this migration without the need to create another MR or temporarily remove this condition in the current MR.
- Show closed items
Activity
-
Newest first Oldest first
-
Show all activity Show comments only Show history only
- Maintainer
@dgruzd We already do this in https://gitlab.com/gitlab-org/database-team/gitlab-com-database-testing/-/blob/master/docker/gitlab/patches/0003-Force-Gitlab-com-true.patch.
Is it not working as expected?
From 11f5d32a22471af67347061198156c030b71cc8a Mon Sep 17 00:00:00 2001 From: Simon Tomlinson <stomlinson@gitlab.com> Date: Mon, 4 Apr 2022 14:38:35 -0500 Subject: [PATCH] Patch to force Gitlab.com? == true --- lib/gitlab.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/gitlab.rb b/lib/gitlab.rb index d33120575a2..4f6d37a625d 100644 --- a/lib/gitlab.rb +++ b/lib/gitlab.rb @@ -56,8 +56,7 @@ def self.simulate_com? end def self.com? - # Check `gl_subdomain?` as well to keep parity with gitlab.com - simulate_com? || Gitlab.config.gitlab.url == Gitlab::Saas.com_url || gl_subdomain? + true end def self.com -- 2.35.1
Collapse replies - Author Developer
@krasio I might have missed that because in one of my MRs we had to create a separate MR with the changes. Thank you!
- Maintainer
@dgruzd Yeah, I've seen this in other MRs too. Wonder if all is working as expected.
/cc @stomlinson
- Maintainer
@dgruzd could you point me to the MR where
Gitlab.com?
wasn't returning true during migration testing? I'll check it out. Thanks! - Maintainer
Here's one example I just saw - gitlab-org/gitlab!89318 (comment 1038468309). Not sure if it has to be done, or there is some confusion around it.
- Author Developer
@stomlinson I've noticed it in gitlab-org/gitlab!88545 (merged)
- Maintainer
For gitlab-org/gitlab!88545 (merged)
It looks like the stubbing is happening correctly to me. We skip the migration if
Gitlab.com?
is truthy, and it looks like the migration is getting skipped.def skip_migration? Gitlab.staging? || Gitlab.com? end
The most recent testing job I see there is https://ops.gitlab.net/gitlab-com/database-team/gitlab-com-database-testing/-/jobs/7266827, with an extremely short runtime:
main: == 20220601110011 ScheduleRemoveSelfManagedWikiNotes: migrating =============== main: == 20220601110011 ScheduleRemoveSelfManagedWikiNotes: migrated (0.0022s) ======
For gitlab-org/gitlab!89318 (comment 1038468309)
Looks like they're commenting out the
return if Gitlab.com?
check (since it would skip their migration) during testing.So for both of these cases, I think the stub is working correctly and making
Gitlab.com?
return true.(We actually have a test for this - during self-test runs of the pipeline, it injects this migration: https://gitlab.com/gitlab-org/database-team/gitlab-com-database-testing/-/blob/master/docker/gitlab/patches/testing/Migration-that-requires-Gitlab-com-true.patch)
1 - Maintainer
Looks like they're commenting out the
return if Gitlab.com?
check (since it would skip their migration) during testing.@stomlinson Right, never mind me, I read it totally wrong.
- Dmitry Gruzd closed
closed