Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion service/Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
agama-yast (19)
agama-yast (19.devel14.752a116e9)
cfa (~> 1.0.2)
cfa_grub2 (~> 2.0.0)
cheetah (~> 1.0.0)
Expand Down
67 changes: 65 additions & 2 deletions service/lib/agama/autoyast/profile_fetcher.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,28 @@ module AutoYaST
#
# It generates an AutoYaST profile that can be converted into an Agama configuration using the
# {Agama::AutoYaST::Converter} class.
#
# # Removing empty strings
#
# It is really uncommon to use empty strings as values in AutoYaST.
# Those values are not a problem in AutoYaST, but they can cause
# problems in Agama. For instance, an empty <keymap /> element is ignored
# in AutoYaST, but not in Agama.
#
# In order to make the migration from AutoYaST to Agama easier, the
# conversion removes empty string values. The exception is the
# <partitioning /> section, which is handled by yast2-storage-ng.
# Actually, this section contains the <subvolumes_prefix />, which
# can be set to an empty string as described in the AutoYaST documentation.
#
class ProfileFetcher
# @param profile_url [String] Profile URL
def initialize(profile_url)
@profile_url = profile_url
end

# Converts the profile into a set of files that Agama can process.
# @return [Hash,nil] an evaluated AutoYaST profile
# @return [ProfileHash,nil] an evaluated AutoYaST profile
def fetch
import_yast
read_profile
Expand Down Expand Up @@ -71,7 +85,7 @@ def read_profile
FileUtils.rm(Yast::AutoinstConfig.modified_profile)
end

Yast::ProfileHash.new(Yast::Profile.current)
Yast::ProfileHash.new(clean_profile(Yast::Profile.current))
end

def run_pre_scripts
Expand All @@ -94,6 +108,55 @@ def tmp_profile_path
)
end

# Removes empty string values from the profile.
#
# @param profile [Hash]
# @return [Hash]
def clean_profile(profile)
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: in theory there is place for de-duplication ... the profile is implemented as a Hash ... so clean_profile and clean_hash are kind of duplicates (I'm aware of that "partitioning" condition)

Copy link
Copy Markdown
Contributor

@mchf mchf Mar 19, 2026

Choose a reason for hiding this comment

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

I mean in theory clean_profile is something like:

clean_hash(profile.except("partitioning")).merge(profile["partitioning"])

or something like that

Copy link
Copy Markdown
Contributor Author

@imobachgs imobachgs Mar 19, 2026

Choose a reason for hiding this comment

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

Well, that code will crash (as profile["partitioning"] could be nil), but I get the idea. However, you still need some place to implement that logic. Not to mention that we would like to avoid hashes that turn into nil (because they are empty).

profile.each_with_object({}) do |(key, value), all|
if key == "partitioning"
all[key] = value
else
new_value = clean_value(value)
all[key] = new_value unless new_value.nil?
end
end
end

def clean_value(value)
case value
when Array
clean_array(value)

when Hash
clean_hash(value)

when String
value unless value.empty?

else
value
end
end

def clean_array(array)
array.map do |e|
if e.is_a?(Hash)
clean_value(e)
else
e
end
end
end

def clean_hash(hash)
new_hash = hash.each_with_object({}) do |(key, value), all|
new_value = clean_value(value)
all[key] = new_value unless new_value.nil?
end
new_hash unless new_hash.empty?
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.

if my internal parser works correctly this leads to filtering the key / value pair out in clean_profile so in general empty hashes are not converted. I'm fine with that but do not understand why the same doesn't happen for arrays (clean_array doesn't care about empty arrays)

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.

Because an empty array can be used to remove some predefined (like a list of subvolumes).

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.

that is already covered by not doing filtering on storage section at all, isn't it? Any other example? I'm just curious.

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.

That's the point. I am not 100% sure if it happens in any other place. So I prefer to be conservative.

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.

Having said that, we could change that, but for sure not now (it needs further testing and we are already late :-)).

end

def import_yast
Yast.import "AutoinstConfig"
Yast.import "AutoinstScripts"
Expand Down
6 changes: 6 additions & 0 deletions service/package/rubygem-agama-yast.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Wed Mar 18 23:44:14 UTC 2026 - Imobach Gonzalez Sosa <igonzalezsosa@suse.com>

- Remove empty strings when importing an AutoYaST profile
(gh#agama-project/agama#3302).

-------------------------------------------------------------------
Tue Mar 17 12:15:21 UTC 2026 - Imobach Gonzalez Sosa <igonzalezsosa@suse.com>

Expand Down
26 changes: 26 additions & 0 deletions service/test/agama/autoyast/profile_fetcher_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -147,4 +147,30 @@
subject.fetch
end
end

context "when the profile contains empty strings as values" do
let(:profile_name) { "empty-values.xml" }

it "removes them if they are not in the \"partitioning\" section" do
result = subject.fetch

user = result["users"].first
expect(user["username"]).to eq("root")
expect(user).to_not have_key("password")

language = result["language"]
expect(language).to have_key("language")
expect(language).to_not have_key("languages")

drive = result["partitioning"].first
partition = drive["partitions"].first
expect(partition).to have_key("subvolumes_prefix")
end

it "removes empty sections" do
result = subject.fetch

expect(result).to_not have_key("keyboard")
end
end
end
59 changes: 59 additions & 0 deletions service/test/fixtures/profiles/empty-values.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE profile>
<profile xmlns="http://www.suse.com/1.0/yast2ns" xmlns:config="http://www.suse.com/1.0/configns">
<software>
<products config:type="list">
<product>Tumbleweed</product>
</products>
<patterns config:type="list">
<pattern>enhanced_base</pattern>
</patterns>
</software>

<language>
<language>es_ES.UTF-8</language>
<languages></languages>
</language>

<timezone>
<timezone>Atlantic/Canary</timezone>
</timezone>

<keyboard>
<keymap></keymap>
</keyboard>

<partitioning t="list">
<drive t="map">
<device>/dev/system</device>
<enable_snapshots t="boolean">true</enable_snapshots>
<partitions t="list">
<partition t="map">
<create t="boolean">true</create>
<create_subvolumes t="boolean">true</create_subvolumes>
<filesystem t="symbol">btrfs</filesystem>
<format t="boolean">true</format>
<subvolumes_prefix><![CDATA[]]></subvolumes_prefix>
</partition>
</partitions>
</drive>
</partitioning>

<users config:type="list">
<user>
<username>root</username>
<encrypted config:type="boolean">false</encrypted>
<user_password></user_password>
<authorized_keys config:type="list">
<listentry>ssh-rsa ...</listentry>
</authorized_keys>
</user>
<user>
<username>jane</username>
<uid>1000</uid>
<fullname>Jane Doe</fullname>
<encrypted config:type="boolean">false</encrypted>
<user_password>12345678</user_password>
</user>
</users>
</profile>
Loading