Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for openSUSE MicroOS #5998

Merged
merged 6 commits into from
Feb 25, 2023
Merged

Added support for openSUSE MicroOS #5998

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
821d030
fix(zypper): Added condition to check for transactional-update binary…
Feb 16, 2023
4924bfd
style(changelog): Made zypper-change uppercase
andre161292 Feb 17, 2023
a880100
fix(zypper): Removed check for /var/lib/misc/transactional-update.state
Feb 20, 2023
db0b4bd
feat(zypper): Aligned transactional-update checks with zypper's
Feb 22, 2023
9763f52
refactor(zypper): Removed dependency to psutil and made use of parsin…
Feb 22, 2023
1003df2
refactor(zypper): Removed need for regex, plus small refactoring
Feb 22, 2023
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: 2 additions & 0 deletions changelogs/fragments/5615-zypper-transactional-update.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bugfixes:
- "zypper - Make package managing work on readonly filesystem of openSUSE MicroOS (https://github.com/ansible-collections/community.general/pull/5615)."
andre161292 marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 2 additions & 1 deletion plugins/modules/zypper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,8 @@ def repo_refresh(m):


def transactional_updates():
return os.path.exists('/var/lib/misc/transactional-update.state')
return os.path.exists('/var/lib/misc/transactional-update.state') \
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return os.path.exists('/var/lib/misc/transactional-update.state') \
return any(map(os.path.exists, ['/var/lib/misc/transactional-update.state', '/usr/sbin/transactional-update']))

another way to write this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume the suggested change makes the boolean operation non-trivial anymore, in the way that it allocates an array and has to reflectively apply the list items to the supplied function. For more items i'd be with you, but for two..?

Thx for the suggestion nevertheless! Phyton's not my native language and i'm happy to learn about new ways to do stuff :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

On Python 3 it the map result is a generator, but on Python 2 this causes two calls to os.path.exists, while the original statement only makes two calls if the first one returns False.

But using a generator expression would make it rather efficient (though still slower than the original version) even on Python 2.6 and 2.7: any(os.path.exists(path) for path in ('/var/lib/misc/transactional-update.state', '/usr/sbin/transactional-update')).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the explanation 🤗 Makes sense.

So should i change it, or leave it as is? What's preferred?

Copy link

Choose a reason for hiding this comment

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

Python 2 is dead and is probably irrelevant if something there is slower than the supported release. What does /var/lib/misc/transactional-update.state contain? And is the system in a valid state if this file is missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, thanks for pinging me.

Is the pure existence of the executable enough to check if transactional updates are actually in use? It may be true for MicroOS, which only exists in the transaction-variant. In openSUSE (e.g. Tumbleweed) I can as well install transactional-update and have the executable on the disk. That does not mean I actually activated transactional updates. That's the reason why I checked for the state file.

If there's a better way to check if transactional updates are really in use (and not only "possible"), I'm for the change. But not when we break the usage of the module on other systems.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to identify MicroOS? Maybe looking at /etc/os-release or /etc/lsb-release?

Copy link

Choose a reason for hiding this comment

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

@felixfontein in my MicroOS installation there is no /etc/lsb-release file, but /etc/os-release contains:

NAME="openSUSE MicroOS"
# VERSION="20230130"
ID="opensuse-microos"
ID_LIKE="suse opensuse opensuse-tumbleweed"
VERSION_ID="20230130"
PRETTY_NAME="openSUSE MicroOS"
ANSI_COLOR="0;32"
CPE_NAME="cpe:/o:opensuse:microos:20230130"
BUG_REPORT_URL="https://bugs.opensuse.org"
HOME_URL="https://www.opensuse.org/"
DOCUMENTATION_URL="https://en.opensuse.org/Portal:MicroOS"
LOGO="distributor-logo-MicroOS"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe looking at /etc/os-release or /etc/lsb-release?

It's id is opensuse-microos, so yes.
But we shouldn't only rely on that, as the state-file doesn't exist on a transactional Tumbleweed/Leap install.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should replicate the checks zypper uses, if suitable. And it doesn't seem to use black magic, so we should be able to do so: https://github.com/openSUSE/zypper/blob/master/src/commands/conditions.cc#L44

or os.path.exists('/usr/sbin/transactional-update')

# ===========================================
# Main control flow
Expand Down