From f6b34edba5f3d716ff31419b910ad2041c4861be Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Tue, 9 Nov 2021 20:24:31 -0300 Subject: [PATCH 1/2] Fix no repo error message for group-level wikis The replicator.class.git_access_class.error_message(:no_repo) returns the wrong error message for group wiki repositories. The helper method handle the error message for each datatype. Changelog: fixed EE: true --- .../Geo Replicate a new Git repository type.md | 7 +++++++ ee/app/replicators/geo/group_wiki_repository_replicator.rb | 4 ++++ ee/app/replicators/geo/snippet_repository_replicator.rb | 4 ++++ ee/app/services/geo/framework_repository_sync_service.rb | 7 ++++--- .../geo/group_wiki_repository_replicator_spec.rb | 6 ++++++ .../replicators/geo/snippet_repository_replicator_spec.rb | 6 ++++++ .../services/geo/framework_repository_sync_service_spec.rb | 2 ++ .../repository_replicator_strategy_shared_examples.rb | 6 ++++++ 8 files changed, 39 insertions(+), 3 deletions(-) diff --git a/.gitlab/issue_templates/Geo Replicate a new Git repository type.md b/.gitlab/issue_templates/Geo Replicate a new Git repository type.md index 5cb99ebcb17ef4..71a962d178954e 100644 --- a/.gitlab/issue_templates/Geo Replicate a new Git repository type.md +++ b/.gitlab/issue_templates/Geo Replicate a new Git repository type.md @@ -313,6 +313,10 @@ That's all of the required database changes. ::Gitlab::GitAccessCoolWidget end + def self.no_repo_message + git_access_class.error_message(:no_repo) + end + # The feature flag follows the format `geo_#{replicable_name}_replication`, # so here it would be `geo_cool_widget_replication` def self.replication_enabled_by_default? @@ -351,6 +355,9 @@ That's all of the required database changes. ``` - [ ] Make sure a Geo secondary site can request and download Cool Widgets on the Geo primary site. You may need to make some changes to `Gitlab::GitAccessCoolWidget`. For example, see [this change for Group-level Wikis](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/54914/diffs?commit_id=0f2b36f66697b4addbc69bd377ee2818f648dd33). + +- [ ] Make sure a Geo secondary site can replicate Cool Widgets where repository does not exist on the Geo primary site. The only way to know about this is to parse the error text. You may need to make some changes to `Gitlab::CoolWidgetReplicator.no_repo_message` to return the proper error message. For example, see [this change for Group-level Wikis](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/74133). + - [ ] Generate the feature flag definition file by running the feature flag command and following the command prompts: ```shell diff --git a/ee/app/replicators/geo/group_wiki_repository_replicator.rb b/ee/app/replicators/geo/group_wiki_repository_replicator.rb index 66a9cacf977658..7398024f169e49 100644 --- a/ee/app/replicators/geo/group_wiki_repository_replicator.rb +++ b/ee/app/replicators/geo/group_wiki_repository_replicator.rb @@ -12,6 +12,10 @@ def self.git_access_class ::Gitlab::GitAccessWiki end + def self.no_repo_message + git_access_class.error_message(:no_group_repo) + end + def repository model_record.repository end diff --git a/ee/app/replicators/geo/snippet_repository_replicator.rb b/ee/app/replicators/geo/snippet_repository_replicator.rb index 891fda1e054572..8590d6b23e9d7e 100644 --- a/ee/app/replicators/geo/snippet_repository_replicator.rb +++ b/ee/app/replicators/geo/snippet_repository_replicator.rb @@ -13,6 +13,10 @@ def self.git_access_class ::Gitlab::GitAccessSnippet end + def self.no_repo_message + git_access_class.error_message(:no_repo) + end + override :verification_feature_flag_enabled? def self.verification_feature_flag_enabled? true diff --git a/ee/app/services/geo/framework_repository_sync_service.rb b/ee/app/services/geo/framework_repository_sync_service.rb index 1bd8972ed1aa62..aa97fba0be0abc 100644 --- a/ee/app/services/geo/framework_repository_sync_service.rb +++ b/ee/app/services/geo/framework_repository_sync_service.rb @@ -45,9 +45,10 @@ def sync_repository log_info('Expiring caches') repository.after_create rescue Gitlab::Shell::Error, Gitlab::Git::BaseError => e - # In some cases repository does not exist, the only way to know about this is to parse the error text. - # If it does not exist we should consider it as successfully downloaded. - if e.message.include?(replicator.class.git_access_class.error_message(:no_repo)) + # In some cases repository does not exist, the only way to know about this + # is to parse the error text. If it does not exist we should consider it + # as successfully downloaded. + if e.message.include?(replicator.class.no_repo_message) log_info('Repository is not found, marking it as successfully synced') mark_sync_as_successful(missing_on_primary: true) else diff --git a/ee/spec/replicators/geo/group_wiki_repository_replicator_spec.rb b/ee/spec/replicators/geo/group_wiki_repository_replicator_spec.rb index 483135a695c0a3..c49d00dea3ed16 100644 --- a/ee/spec/replicators/geo/group_wiki_repository_replicator_spec.rb +++ b/ee/spec/replicators/geo/group_wiki_repository_replicator_spec.rb @@ -6,4 +6,10 @@ let(:model_record) { build(:group_wiki_repository, group: create(:group)) } include_examples 'a repository replicator' + + describe '.no_repo_message' do + it 'returns the proper error message for group-level wikis' do + expect(replicator.class.no_repo_message).to eq(::Gitlab::GitAccessWiki.error_message(:no_group_repo)) + end + end end diff --git a/ee/spec/replicators/geo/snippet_repository_replicator_spec.rb b/ee/spec/replicators/geo/snippet_repository_replicator_spec.rb index a1073471347327..021a749da30a4e 100644 --- a/ee/spec/replicators/geo/snippet_repository_replicator_spec.rb +++ b/ee/spec/replicators/geo/snippet_repository_replicator_spec.rb @@ -8,4 +8,10 @@ include_examples 'a repository replicator' it_behaves_like 'a verifiable replicator' + + describe '.no_repo_message' do + it 'returns the proper error message for snippet repositories' do + expect(replicator.class.no_repo_message).to eq(::Gitlab::GitAccessSnippet.error_message(:no_repo)) + end + end end diff --git a/ee/spec/services/geo/framework_repository_sync_service_spec.rb b/ee/spec/services/geo/framework_repository_sync_service_spec.rb index 8494fbfdc8c541..b778b0850b56b2 100644 --- a/ee/spec/services/geo/framework_repository_sync_service_spec.rb +++ b/ee/spec/services/geo/framework_repository_sync_service_spec.rb @@ -120,6 +120,8 @@ .with(url_to_repo, forced: true, http_authorization_header: anything) .and_raise(Gitlab::Shell::Error.new(Gitlab::GitAccessSnippet::ERROR_MESSAGES[:no_repo])) + expect(replicator.class).to receive(:no_repo_message).once.and_call_original + subject.execute expect(registry).to have_attributes( diff --git a/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb b/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb index 842146911f6ec8..1f0294ab2e3584 100644 --- a/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb +++ b/ee/spec/support/shared_examples/models/concerns/repository_replicator_strategy_shared_examples.rb @@ -128,6 +128,12 @@ end end + describe '.no_repo_message' do + it 'is implemented' do + expect(replicator.class.no_repo_message).to be_a(String) + end + end + describe '#model' do let(:invoke_model) { replicator.class.model } -- GitLab From b1c553700ffdc10976e48079358e4924c2751c0d Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Wed, 10 Nov 2021 18:37:56 +0000 Subject: [PATCH 2/2] Clarfy code comment --- ee/app/services/geo/framework_repository_sync_service.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ee/app/services/geo/framework_repository_sync_service.rb b/ee/app/services/geo/framework_repository_sync_service.rb index aa97fba0be0abc..95e2beda93287e 100644 --- a/ee/app/services/geo/framework_repository_sync_service.rb +++ b/ee/app/services/geo/framework_repository_sync_service.rb @@ -46,8 +46,9 @@ def sync_repository repository.after_create rescue Gitlab::Shell::Error, Gitlab::Git::BaseError => e # In some cases repository does not exist, the only way to know about this - # is to parse the error text. If it does not exist we should consider it - # as successfully downloaded. + # is to parse the error text. If the repository does not exist on the + # primary, then the state on this secondary matches the primary, and + # therefore the repository is successfully synced. if e.message.include?(replicator.class.no_repo_message) log_info('Repository is not found, marking it as successfully synced') mark_sync_as_successful(missing_on_primary: true) -- GitLab