Fix wrong directory content copying resulting in repodata/repodata

features/mirror_spec.rb

@@ -1,23 +1,37 @@
-require File.expand_path('../support/command_rspec_helper', __FILE__)
+require File.expand_path('support/command_rspec_helper', __dir__)
 
 describe 'mirror' do
-  let(:files) { %w'repomd.xml repomd.xml.asc repomd.xml.key *-primary.xml.gz *-filelists.xml.gz *-other.xml.gz' }
+  let(:path) { '/var/lib/rmt/public/repo/SUSE/Products/SLE-Product-SLES/15-SP5/x86_64' }
 
   before do
-    `/usr/bin/rmt-cli repos enable 3114`
+    `/usr/bin/rmt-cli repos enable 5664`
     `/usr/bin/rmt-cli mirror`
   end
 
   after do
-    `/usr/bin/rmt-cli repos disable 3114`
+    `/usr/bin/rmt-cli repos disable 5664`
 
     # cleanup files
-    FileUtils.rm_r('/var/lib/rmt/public/repo/SUSE/Updates/SLE-Product-SLES/15')
+    FileUtils.rm_r(path)
   end
 
-  it do
-    files.each do |filename_pattern|
-      expect(`find /var/lib/rmt/public/ -name \'#{filename_pattern}\'`).to include(filename_pattern.gsub('*', ''))
+  let(:metadata_files) { %w[repomd.xml repomd.xml.asc repomd.xml.key *-primary.xml.gz *-filelists.xml.gz *-other.xml.gz] }
+
+  it 'has valid metadata mirrored' do
+    metadata_files.each do |filename_pattern|
+      expect(`find #{File.join(path, 'product', 'repodata')} -maxdepth 1 -name \'#{filename_pattern}\'`).to include(filename_pattern.delete('*'))
     end
   end
+
+  let(:license_files) { %w[license.txt directory.yast license.de.txt] }
+
+  it 'has licenses correctly mirrored' do
+    license_files.each do |filename_pattern|
+      expect(`find #{File.join(path, 'product.license')} -maxdepth 1 -name \'#{filename_pattern}\'`).to include(filename_pattern.delete('*'))
+    end
+  end
+
+  it 'has rpms correctly mirrored' do
+    expect(`find #{File.join(path, 'product', 'x86_64')} -maxdepth 1 -name sles-release-*.rpm`).to include('sles-release')
+  end
 end

lib/rmt/mirror/base.rb

@@ -29,6 +29,7 @@ def mirror
     raise RMT::Mirror::Exception.new(_('Error while mirroring repository: %{error}' % { error: e.message }))
   ensure
     cleanup_temp_dirs
+    cleanup_stale_metadata
   end
 
   protected
@@ -130,18 +131,26 @@ def need_to_download?(ref)
     true
   end
 
-  def move_directory(source:, destination:)
-    FileUtils.mv(source, destination, force: true)
-    FileUtils.chmod(0o755, destination)
+  def cleanup_stale_metadata
+    # A bug introduced in 2.16 writes metadata into its own directory if exists having
+    # directory structure like repodata/repodata.
+    # see: https://github.com/SUSE/rmt/issues/1136
+    FileUtils.remove_entry(repository_path('repodata', 'repodata')) if Dir.exist?(repository_path('repodata', 'repodata'))
+
+    # With 1.0.0 a backup mechanism was introduced creating .old_* backups of metadata which was never really used
+    # we remove these files now from the mirrored repositories
+    # see: https://github.com/SUSE/rmt/pull/1120/files#diff-69bc4fdeb7aa7ceab24bec11c65a184357e5b71317125516edfa2d819653a969L131
+    glob_old_backups = Dir.glob(repository_path('.old_*'))
+
+    glob_old_backups.each do |old|
+      FileUtils.remove_entry(old)
+    end
   rescue StandardError => e
-    raise RMT::Mirror::Exception.new(_('Error while moving directory %{src} to %{dest}: %{error}') % {
-      src: source,
-      dest: destination,
-      error: e.message
-    })
+    logger.debug("Can not remove stale metadata directory: #{e}")
   end
 
   def move_files(glob:, destination:)
+    FileUtils.mkpath(destination) unless Dir.exist?(destination)
     FileUtils.mv(Dir.glob(glob), destination, force: true)
   rescue StandardError => e
     raise RMT::Mirror::Exception.new(_('Error while moving files %{glob} to %{dest}: %{error}') % {

lib/rmt/mirror/license.rb

@@ -35,7 +35,8 @@ def mirror_implementation
 
     download_enqueued
 
-    move_directory(source: temp(:license), destination: repository_path)
+    glob_licenses = File.join(temp(:license), '*')
+    move_files(glob: glob_licenses, destination: repository_path)
   rescue RMT::Downloader::Exception => e
     raise RMT::Mirror::Exception.new(_('Error while mirroring license files: %{error}') % { error: e.message })
   end

lib/rmt/mirror/repomd.rb

@@ -6,15 +6,16 @@ def mirror_implementation
     create_repository_path
     create_temp_dir(:metadata)
 
-    if repository_url.ends_with?('product')
+    if repository_url.ends_with?('product/')
       licenses = RMT::Mirror::License.new(repository: repository, logger: logger, mirroring_base_dir: mirroring_base_dir)
       licenses.mirror
     end
 
     metadata_files = mirror_metadata
     mirror_packages(metadata_files)
 
-    move_directory(source: File.join(temp(:metadata), 'repodata'), destination: repository_path('repodata'))
+    glob_metadata = File.join(temp(:metadata), 'repodata', '*')
+    move_files(glob: glob_metadata, destination: repository_path('repodata'))
   end
 
   protected

package/obs/rmt-server.changes

@@ -1,7 +1,8 @@
 Thu April 18 09:27:00 UTC 2024 - Adnilson Delgado <[email protected]>
 
 - Version 2.17 :
-  * Improve CLI mirroring summary information by adding the mirror repositories, the file count and size. This fixes issue #702.
+  * Improve CLI mirroring summary information by adding the mirror repositories, the file count and size. (gh#702)
+  * Copy metadata content to repodata/ and not create a seperate subdirectory repodata/repodata (gh#1136)
 
 -------------------------------------------------------------------
 Thu April 17 15:22:00 UTC 2024 - Yogalakshmi Arunachalam <[email protected]>

spec/lib/rmt/mirror/base_spec.rb

@@ -36,12 +36,14 @@
     it 'mirrors repositories and licenses' do
       expect(base).to receive(:mirror_implementation)
       expect(base).to receive(:cleanup_temp_dirs)
+      expect(base).to receive(:cleanup_stale_metadata)
       base.mirror
     end
 
     it 'throws an exception if mirroring fails' do
       allow(base).to receive(:mirror_implementation).and_raise(RMT::Mirror::Exception)
       expect(base).to receive(:cleanup_temp_dirs)
+      expect(base).to receive(:cleanup_stale_metadata)
       expect { base.mirror }.to raise_error(RMT::Mirror::Exception)
     end
   end
@@ -200,35 +202,27 @@
     end
   end
 
-  describe '#move_directory' do
-    let(:src) { '/source/path' }
+  describe '#move_files' do
+    let(:src) { '/source/path/*' }
     let(:dest) { '/destination/path' }
-    let(:backup) { '/destination/.backup_path' }
-
-    it 'moves content from source to destination' do
-      expect(FileUtils).to receive(:mv).with(src, dest, force: true)
-      expect(FileUtils).to receive(:chmod).with(0o755, dest)
 
-      base.move_directory(source: src, destination: dest)
-    end
-
-    it 'fails on file system errors' do
-      allow(FileUtils).to receive(:mv).with(src, dest, force: true).and_raise(StandardError)
+    it 'creates the destination directory if not yet existing' do
+      allow(Dir).to receive(:exist?).with(dest).and_return(false)
+      expect(FileUtils).to receive(:mkpath).with(dest)
+      expect(FileUtils).to receive(:mv).with(Dir.glob(src), dest, force: true)
 
-      expect { base.move_directory(source: src, destination: dest) }.to raise_exception(/Error while moving directory/)
+      base.move_files(glob: src, destination: dest)
     end
-  end
-
-  describe '#move_files' do
-    let(:src) { '/source/path' }
-    let(:dest) { '/destination/path' }
 
-    it 'copies content from source to destination without backup' do
+    it 'copies content from source to destination' do
+      allow(Dir).to receive(:exist?).with(dest).and_return(true)
       expect(FileUtils).to receive(:mv).with(Dir.glob(src), dest, force: true)
+
       base.move_files(glob: src, destination: dest)
     end
 
     it 'fails on file system errors' do
+      allow(Dir).to receive(:exist?).with(dest).and_return(true)
       allow(FileUtils).to receive(:mv).with(Dir.glob(src), dest, force: true).and_raise(StandardError)
 
       expect { base.move_files(glob: src, destination: dest) }.to raise_exception(/Error while moving files/)
@@ -286,4 +280,62 @@
       expect(base.need_to_download?(package)).to be(false)
     end
   end
+
+  describe '#cleanup_stale_metadata' do
+    let(:duplicated_repodata_path) { base.repository_path('repodata', 'repodata') }
+    let(:old_backup_glob) { base.repository_path('.old_*') }
+    let(:found_old_backup_files) do
+      [
+        base.repository_path('.old_repodata'),
+        base.repository_path('.old_license')
+      ]
+    end
+
+    context 'with repodata/repodata/' do
+      it 'removes the directory' do
+        allow(Dir).to receive(:exist?).with(duplicated_repodata_path).and_return(true)
+        allow(Dir).to receive(:glob).with(old_backup_glob).and_return([])
+
+        expect(FileUtils).to receive(:remove_entry).with(duplicated_repodata_path)
+
+        base.cleanup_stale_metadata
+      end
+    end
+
+    context 'with .old_repodata' do
+      it 'removes stale .old_* backups' do
+        allow(Dir).to receive(:exist?).with(duplicated_repodata_path).and_return(false)
+        allow(Dir).to receive(:glob).with(old_backup_glob).and_return(found_old_backup_files)
+
+        expect(FileUtils).to receive(:remove_entry).with(found_old_backup_files[0])
+        expect(FileUtils).to receive(:remove_entry).with(found_old_backup_files[1])
+
+        base.cleanup_stale_metadata
+      end
+    end
+
+    context 'filesystem error' do
+      it 'does not raise' do
+        allow(Dir).to receive(:exist?).with(duplicated_repodata_path).and_return(false)
+        allow(Dir).to receive(:glob).with(old_backup_glob).and_return(found_old_backup_files)
+        allow(FileUtils).to receive(:remove_entry).with(found_old_backup_files[1]).and_raise(StandardError)
+
+        expect(FileUtils).to receive(:remove_entry).with(found_old_backup_files[0])
+        expect(base.logger).to receive(:debug)
+
+        base.cleanup_stale_metadata
+      end
+    end
+
+    context 'without stale metadata' do
+      it 'does not delete any directory' do
+        allow(Dir).to receive(:exist?).with(duplicated_repodata_path).and_return(false)
+        allow(Dir).to receive(:glob).with(old_backup_glob).and_return([])
+
+        expect(FileUtils).not_to receive(:remove_entry)
+
+        base.cleanup_stale_metadata
+      end
+    end
+  end
 end

spec/lib/rmt/mirror/license_spec.rb

@@ -67,7 +67,7 @@
       allow(license).to receive(:download_cached!).with('directory.yast', to: '/tmp/foo').and_return(licenses_ref)
       expect(license).to receive(:download_enqueued)
       expect(license).to receive(:enqueue).with(duck_type(:local_path)).exactly(11).times
-      expect(license).to receive(:move_directory).with(source: license.temp(:license), destination: license.repository_path)
+      expect(license).to receive(:move_files).with(glob: File.join(license.temp(:license), '*'), destination: license.repository_path)
 
       license.mirror_implementation
     end

spec/lib/rmt/mirror/repomd_spec.rb

@@ -5,8 +5,8 @@
 
   let(:logger) { RMT::Logger.new('/dev/null') }
 
-
-  let(:repository_url) { 'https://updates.suse.com/sample/repository/15.4/product' }
+  # Remember that RMT forces a trialing slash on all repository URLS!
+  let(:repository_url) { 'https://updates.suse.com/sample/repository/15.4/product/' }
   let(:repository) do
     create :repository,
            name: 'SUSE Linux Enterprise Server 15 SP4',
@@ -52,7 +52,7 @@
       expect(licenses).to receive(:mirror)
       expect(repomd).to receive(:mirror_metadata)
       expect(repomd).to receive(:mirror_packages)
-      expect(repomd).to receive(:move_directory).with(source: 'a/repodata', destination: repomd.repository_path('repodata'))
+      expect(repomd).to receive(:move_files).with(glob: 'a/repodata/*', destination: repomd.repository_path('repodata'))
 
       repomd.mirror_implementation
     end
@@ -65,7 +65,7 @@
         expect(repomd).to receive(:create_temp_dir).with(:metadata)
         expect(repomd).to receive(:mirror_metadata)
         expect(repomd).to receive(:mirror_packages)
-        expect(repomd).to receive(:move_directory).with(source: 'a/repodata', destination: repomd.repository_path('repodata'))
+        expect(repomd).to receive(:move_files).with(glob: 'a/repodata/*', destination: repomd.repository_path('repodata'))
 
         expect(licenses).not_to receive(:mirror)