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 warn to ensure that system is fully upgraded (RhBug:1225442) #97

Merged
merged 5 commits into from
Jul 26, 2017

Conversation

j-mracek
Copy link
Contributor

@j-mracek j-mracek commented Jun 8, 2017

It should help user to realize that there could be an issue when system is not
fully upgraded.

The patch also enhance the documentation.

https://bugzilla.redhat.com/show_bug.cgi?id=1225442

Requires: rpm-software-management/dnf#841

release. It replaces fedup (the old Fedora Upgrade tool).

release. It replaces fedup (the old Fedora Upgrade tool). Before you proceed ensure that your system
is fully upgraded (``dnf upgrade --refresh``).
Copy link
Member

Choose a reason for hiding this comment

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

It should be dnf --refresh upgrade. Technically the order doesn't matter, but that's how the man page displays it, so we should be consistent.

@@ -345,6 +345,8 @@ def _call_sub(self, name, *args):
# == configure_*: set up action-specific demands ==========================

def configure_download(self, *args):
msg = _('Please ensure that your system is fully upgraded!')
logger.warning(self.base.output.term.bold(msg) + ' ("dnf upgrade --refresh")')
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@@ -111,6 +111,8 @@ Examples
Typical upgrade usage
---------------------

``dnf upgrade --refresh``
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@j-mracek
Copy link
Contributor Author

I would like to add some commits

@j-mracek
Copy link
Contributor Author

I am going to remove some lines

@j-mracek j-mracek force-pushed the system-upgrade branch 2 times, most recently from f6a9f05 to 4694e64 Compare June 22, 2017 10:06
@j-mracek
Copy link
Contributor Author

There is one disadvantage of the approach presented here, if anyone runs successful transaction after download, system upgrade will loose cached files and transaction will fail during reboot.
I have also a plan to remove the upgrade during reboot functionality from system upgrade and provide it to other commands like install, upgrade (requested here https://bugzilla.redhat.com/show_bug.cgi?id=1382063).

Copy link

@dmach dmach left a comment

Choose a reason for hiding this comment

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

The overall code looks good.
It's great simplification over the original code.

Could you consider changing couple coding style issues?


import dnf
from rpmconf import rpmconf
import errno
import sys
Copy link

Choose a reason for hiding this comment

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

Change the import order according to PEP-8:
system imports, 3rd party library imports, project imports

os.unlink(fullpath)
except OSError:
pass
if os.path.isdir(path):
Copy link

Choose a reason for hiding this comment

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

The code is fine, but I'd rather do:

if not os.path.isdir(path):
    return
<remaining code>

def configure_download(self):
if self.base._promptWanted():
msg = _('Do not continue unless your system is fully upgraded '
'("dnf --refresh upgrade")! Do you want to continue')
Copy link

Choose a reason for hiding this comment

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

Maybe improve the message? :
Before you continue, make sure your system is fully upgraded by running "dnf --refresh upgrade".
Do you want to continue?

import dnf
import dnfpluginsextras
import sys

Copy link

Choose a reason for hiding this comment

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

import order again

from dnf.cli import CliError

import json
import os
import uuid

Copy link

Choose a reason for hiding this comment

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

import order again

@@ -17,16 +17,15 @@
# License and may only be used or replicated with the express permission of
# Red Hat, Inc.
#
from dnfpluginsextras import _, logger

Copy link

Choose a reason for hiding this comment

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

import order again

@@ -21,16 +21,15 @@
#

from __future__ import absolute_import
from dnfpluginsextras import _
from tracer import Query, Package
from tracer.views.default import DefaultView
Copy link

Choose a reason for hiding this comment

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

import order again

@j-mracek
Copy link
Contributor Author

@dmach Here is an improved version

It should help user to realize that there could be an issue when system is not
fully upgraded.

The patch also enhance the documentation.

https://bugzilla.redhat.com/show_bug.cgi?id=1225442
It reflects that system-upgrade is plugin of extras.
The former behavior of system-upgrade installs all packages from given
directory, but it looses information about origin of packages. The present
behavior uses directly cached packages in repo, that also allows to use local
repositories. Unfortunately the problems remains with usage of --datadir option
and cannot be fixed, therefore it will be better to not use it at all.
The option --datadir is also redundant with conf option to redirect of dnf
cache.
@dmach
Copy link

dmach commented Jul 26, 2017

I've reviewed the code and it looks good to me. Merging.

@dmach dmach merged commit 8d8dcdf into rpm-software-management:master Jul 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants