Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 42 additions & 4 deletions service/lib/agama/software/manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -201,10 +201,8 @@ def finish
progress.step(_("Writing repositories to the target system")) do
Yast::Pkg.SourceSaveAll
Yast::Pkg.TargetFinish
# FIXME: Pkg.SourceCacheCopyTo works correctly only from the inst-sys
# (original target "/"), it does not work correctly when using
# "chroot" /run/agama/zypp, it needs to be reimplemented :-(
# Yast::Pkg.SourceCacheCopyTo(Yast::Installation.destdir)
# copy the libzypp caches to the target
copy_zypp_to_target
registration.finish
end
end
Expand Down Expand Up @@ -503,6 +501,46 @@ def pattern_exist?(pattern_name)
!Y2Packager::Resolvable.find(kind: :pattern, name: pattern_name).empty?
end

# this reimplements the Pkg.SourceCacheCopyTo call which works correctly
# only from the inst-sys (it copies the data from "/" where is actually
# the Live system package manager)
# @see https://github.com/yast/yast-pkg-bindings/blob/3d314480b70070299f90da4c6e87a5574e9c890c/src/Source_Installation.cc#L213-L267
def copy_zypp_to_target
# copy the zypp "raw" cache
cache = File.join(TARGET_DIR, "/var/cache/zypp/raw")
if Dir.exist?(cache)
target_cache = File.join(Yast::Installation.destdir, "/var/cache/zypp")
FileUtils.mkdir_p(target_cache)
FileUtils.cp_r(cache, target_cache)
end

# copy the "solv" cache but skip the "@System" directory because it
# contains empty installed packages (there were no installed packages
# before moving the target to "/mnt")
solv_cache = File.join(TARGET_DIR, "/var/cache/zypp/solv")
target_solv = File.join(Yast::Installation.destdir, "/var/cache/zypp/solv")
solvs = Dir.entries(solv_cache) - [".", "..", "@System"]
solvs.each do |s|
FileUtils.cp_r(File.join(solv_cache, s), target_solv)
end

# copy the zypp credentials if present
credentials = File.join(TARGET_DIR, "/etc/zypp/credentials.d")
if Dir.exist?(credentials)
target_credentials = File.join(Yast::Installation.destdir, "/etc/zypp")
FileUtils.mkdir_p(target_credentials)
FileUtils.cp_r(credentials, target_credentials)
end

# copy the global credentials if present
glob_credentials = File.join(TARGET_DIR, "/etc/zypp/credentials.cat")
return unless File.exist?(glob_credentials)

target_dir = File.join(Yast::Installation.destdir, "/etc/zypp")
FileUtils.mkdir_p(target_dir)
FileUtils.copy(glob_credentials, target_dir)
end

# update the zypp repositories for the new product, either delete them
# or keep them untouched
# @param new_product [Agama::Software::Product] the new selected product
Expand Down
8 changes: 8 additions & 0 deletions service/package/rubygem-agama-yast.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
-------------------------------------------------------------------
Wed Jun 19 06:04:46 UTC 2024 - Ladislav Slezák <lslezak@suse.com>

- Use a different libzypp target for Agama, do not use the Live
system package management (gh#openSUSE/agama#1329)
- Properly delete the libzypp cache when changing the products
(gh#openSUSE/agama#1349)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK so shouldn't there be an entry for this PR too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it is actually a fix up for #1329 so I do not think it needs any special entry.

-------------------------------------------------------------------
Thu Jun 13 10:53:27 UTC 2024 - Imobach Gonzalez Sosa <igonzalezsosa@suse.com>

Expand Down
45 changes: 45 additions & 0 deletions service/test/agama/software/manager_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@
allow(Agama::Software::RepositoriesManager).to receive(:new).and_return(repositories)
allow(Agama::Software::Proposal).to receive(:new).and_return(proposal)
allow(Agama::ProductReader).to receive(:new).and_call_original
allow(FileUtils).to receive(:mkdir_p)
allow(FileUtils).to receive(:rm_rf)
allow(FileUtils).to receive(:cp_r)
allow(File).to receive(:exist?).and_call_original
end

describe "#new" do
Expand Down Expand Up @@ -352,11 +356,52 @@

describe "#finish" do
it "releases the packaging system" do
allow(subject).to receive(:copy_zypp_to_target)
expect(Yast::Pkg).to receive(:SourceSaveAll)
expect(Yast::Pkg).to receive(:TargetFinish)

subject.finish
end

it "copies the libzypp cache and credentials to the target system" do
allow(Dir).to receive(:exist?).and_call_original

# copying the raw cache
expect(Dir).to receive(:exist?).with("/run/agama/zypp/var/cache/zypp/raw").and_return(true)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np: Although we might consider it good enough, this test just makes sure that you call the expected methods. What about a test that actually performs the copy and checks the result?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I do not like unit tests which really touch the system. For integration tests it is OK but for unit tests I'd rather avoid that if not necessary. And it actually means you are testing the Ruby functionality, not our code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can understand that, but I would say that we are not writing unit tests "by the book" and abusing mocks can produce tests that are not that useful after all. Actually, that test knows a lot about implementation details, which looks wrong to me.

However, as said, I will not block this PR just because of that.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. In such case the test just tests the mocks themselves.

But if we test the real result then we are actually testing the external code and the test is more fragile. If the code basically only calls external code and nothing else then I think it is acceptable.

expect(FileUtils).to receive(:mkdir_p).with("/mnt/var/cache/zypp")
expect(FileUtils).to receive(:cp_r).with("/run/agama/zypp/var/cache/zypp/raw",
"/mnt/var/cache/zypp")

# copy the solv cache
repo_alias = "https-download.opensuse.org-94cc89aa"
expect(Dir).to receive(:entries).with("/run/agama/zypp/var/cache/zypp/solv")
.and_return([".", "..", "@System", repo_alias])
expect(FileUtils).to receive(:cp_r).with(
File.join("/run/agama/zypp/var/cache/zypp/solv/", repo_alias),
"/mnt/var/cache/zypp/solv"
)
# ensure the @System cache is not copied
expect(FileUtils).to_not receive(:cp_r).with(
"/run/agama/zypp/var/cache/zypp/solv/@System",
"/mnt/var/cache/zypp/solv"
)

# copying the credentials.d directory
expect(Dir).to receive(:exist?).with("/run/agama/zypp/etc/zypp/credentials.d")
.and_return(true)
expect(FileUtils).to receive(:mkdir_p).with("/mnt/etc/zypp")
expect(FileUtils).to receive(:cp_r).with("/run/agama/zypp/etc/zypp/credentials.d",
"/mnt/etc/zypp")

# copying the global credentials file
expect(File).to receive(:exist?).with("/run/agama/zypp/etc/zypp/credentials.cat")
.and_return(true)
expect(FileUtils).to receive(:mkdir_p).with("/mnt/etc/zypp")
expect(FileUtils).to receive(:copy).with("/run/agama/zypp/etc/zypp/credentials.cat",
"/mnt/etc/zypp")

subject.finish
end
end

describe "#package_installed?" do
Expand Down