Skip to content

Conversation

@teclator
Copy link
Contributor

@teclator teclator commented May 6, 2025

Problem

The S390x network devices are not activated after the installation because the udev rules are not copying properly to the target system as the relative target path is wrong (/mntetc/udev/rules.d).

Solution

Use File.join to ensure the relative target path and the destdir are joined properly.

Testing

  • Adapted unit test to fail with the original implementation and fix the code for passing
  • Tested manually in agama1
agama1:~ # ls /mnt/etc/udev/rules.d/
41-cio-ignore.rules  41-dasd-eckd-0.0.0160.rules  41-qeth-0.0.0800.rules

@teclator teclator requested review from ancorgs and imobachgs May 6, 2025 06:20
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

It looks good as a quick fix (which is already tested). I wonder why we do not need to update the tests, though.

For the future, I guess that working with Pathname objects instead of strings might be a better alternative.

dgdavid

This comment was marked as outdated.

@dgdavid

This comment was marked as resolved.

@teclator
Copy link
Contributor Author

teclator commented May 6, 2025

Would be nice to have a unit test for this, but I've checked the whole file test is missing. Since you have tested it manually, go ahead 👍

There is a unit test but unfortunately with the happy path, added end "/" for making it break without the proper fix. Now all of the possible combinations should be equivalent.

irb(main):010> [File.join("/mnt", "/etc/udev/rules.d"), File.join("/mnt/", "/etc/udev/rules.d"), File.join("/mnt/", "etc/udev/rules.d"), File.join("/mnt", "etc/udev/rules.d")].uniq
=> ["/mnt/etc/udev/rules.d"]

@dgdavid
Copy link
Contributor

dgdavid commented May 6, 2025

Would be nice to have a unit test for this, but I've checked the whole file test is missing. Since you have tested it manually, go ahead 👍

There is a unit test

Ah, I see. I was looking in the wrong place 😅

@teclator teclator merged commit 952ad27 into beta4 May 6, 2025
5 of 7 checks passed
@teclator teclator deleted the fix_target_path branch May 6, 2025 07:39
@teclator teclator mentioned this pull request May 6, 2025
teclator added a commit that referenced this pull request May 14, 2025
It merges #2301 and #2328 into master
@imobachgs imobachgs mentioned this pull request May 26, 2025
imobachgs added a commit that referenced this pull request May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants