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.

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

Add detection for ostree-based systems and warn users about losing changes #2053

Merged
merged 1 commit into from Apr 24, 2024

Conversation

dcantrell
Copy link
Contributor

On ostree-based systems, users can use dnf to customize the environment but those changes will be lost at the next ostree-based image update. If you want to retain changes between ostree-updates you need to make use of rpm-ostree right now.

All this patch does is add a function to detect systems managed this way and present warnings before dnf operations that modify the system. I display the warning at the beginning of the output, but maybe that should be after any other output is on the display.

@jan-kolarik jan-kolarik self-assigned this Feb 19, 2024
@jan-kolarik
Copy link
Member

I have two proposals regarding the code I've reviewed.

Firstly, as you suggested, I'd go with displaying the warning text to the user during the transaction confirmation, which involves moving the logic before this specific line. This approach eliminates the need to handle individual commands, as it addresses any transactional operation.

Secondly, in util.py, I'd retain only the logic related to detecting container presence. After confirming that is_container returns True, we can relocate the logging of the ostree message to the previously mentioned location. What do you think about that?

@@ -157,6 +158,8 @@ def list_alias(self, cmd):
print(_("Alias %s='%s'") % (cmd, " ".join(args)))

def run(self):
dnf.util.is_container()
Copy link
Member

@j-mracek j-mracek Feb 27, 2024

Choose a reason for hiding this comment

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

Do we want to spread the message to all commands including commands from plugins (dnf-plugins-core, extras, third party), or do we want to be selective?

Copy link
Contributor Author

@dcantrell dcantrell Apr 18, 2024

Choose a reason for hiding this comment

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

I have gone back and forth on this and I think it's mostly wasted effort to be selective right now. I think it's a better idea to do a blanket warning and then shift the focus to incorporating rpm-ostree functionality directly in to native dnf (or a dnf core plugin) where appropriate and then eventually we can just drop this change. I view this change as a temporary thing just to make it nicer for users.

@dcantrell
Copy link
Contributor Author

I have two proposals regarding the code I've reviewed.

Firstly, as you suggested, I'd go with displaying the warning text to the user during the transaction confirmation, which involves moving the logic before this specific line. This approach eliminates the need to handle individual commands, as it addresses any transactional operation.

Secondly, in util.py, I'd retain only the logic related to detecting container presence. After confirming that is_container returns True, we can relocate the logging of the ostree message to the previously mentioned location. What do you think about that?

I like these suggestions, thanks! I will get the PR updated. I've also got an additional method for reliably detecting an ostree system, so I'm going to try to rework is_container() a bit too.

…anges

On ostree-based systems, users can use dnf to customize the
environment but those changes will be lost at the next ostree-based
image update.  If you want to retain changes between ostree-updates
you need to make use of rpm-ostree right now.

Signed-off-by: David Cantrell <[email protected]>
@jan-kolarik
Copy link
Member

LGTM, thanks!

@jan-kolarik jan-kolarik merged commit 5c050ba into rpm-software-management:master Apr 24, 2024
6 of 7 checks passed
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.

None yet

3 participants