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

Don't print pkg that triggered scriptlet as pkg whose the scriptlet is (RhBug:1724779) #1471

Closed

Conversation

kontura
Copy link
Contributor

@kontura kontura commented Aug 30, 2019

No description provided.

@packit-as-a-service
Copy link

There was an error while running a copr build. You can re-trigger copr build by adding a comment (/packit copr-build) into this pull request.

dnf/yum/rpmtrans.py Outdated Show resolved Hide resolved
dnf/yum/rpmtrans.py Outdated Show resolved Hide resolved
dnf/yum/rpmtrans.py Outdated Show resolved Hide resolved
@packit-as-a-service
Copy link

There was an error while running a copr build. You can re-trigger copr build by adding a comment (/packit copr-build) into this pull request.

dnf/yum/rpmtrans.py Outdated Show resolved Hide resolved
@lukash lukash removed their assignment Sep 13, 2019
@j-mracek j-mracek self-assigned this Sep 13, 2019
@@ -409,6 +414,12 @@ def _script_start(self, key):
complete = self.complete_actions if self.total_actions != 0 and self.complete_actions != 0 \
else 1
total = self.total_actions if self.total_actions != 0 and self.complete_actions != 0 else 1

# In case rpm trigger runs scriptlet of some other package that is not in the transaction
if isinstance(key, str) and key != pkg.name:
Copy link

Choose a reason for hiding this comment

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

use dnf.util.is_string_type() instead

# In case rpm trigger runs scriptlet of some other package that is not in the transaction
if isinstance(key, str) and key != pkg.name:
# TODO(amatej): This is bad I think, but we have only the name :(
pkg = self.base._sack.query().installed().filterm(name=key)[0]
Copy link

Choose a reason for hiding this comment

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

_sack() returns list of installed packages that were installed before the transactions
it's not changing during the transaction

consider just printing the package name without any lookups

dnf/yum/rpmtrans.py Outdated Show resolved Hide resolved
dnf/yum/rpmtrans.py Outdated Show resolved Hide resolved
dnf/yum/rpmtrans.py Outdated Show resolved Hide resolved
dnf/yum/rpmtrans.py Outdated Show resolved Hide resolved
@kontura
Copy link
Contributor Author

kontura commented Oct 17, 2019

Updated the PR, it now only prints the name instead of any lookups.

@j-mracek
Copy link
Member

@please could you make the header of commit message shorter. Some information could be moved into body of message.

pkg_currenly_being_handled_name))
else:
msg = ("Error in %s scriptlet in rpm package %s"
% (scriptlet_name, pkg_currenly_being_handled_name))
Copy link
Member

Choose a reason for hiding this comment

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

I do not understand why we do not provide full NEVRA when it is valid and available. Think that you install two kernels in transaction. And one fails but which one?

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 updated it to print the full NEVRA for the current package which has the information always available.

@kontura kontura force-pushed the running_scriptlet branch 2 times, most recently from 8f4f46e to 74a597b Compare October 21, 2019 10:03
@kontura
Copy link
Contributor Author

kontura commented Oct 21, 2019

I also amended the commit message

if pkg_which_owns_this_scriptlet_name != pkg_currenly_being_handled.name:
# Show only the pkg name, because we don't have the full nevra
# and if we search for the pkg we could show misleding information
pkg = pkg_which_owns_this_scriptlet_name
Copy link
Member

Choose a reason for hiding this comment

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

The name of "pkg" is confusing. It is string or pkg object or key (mostly string). What about pkg_or_key?

Copy link
Member

Choose a reason for hiding this comment

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

In progress function it is called package. Therefore not much of help. pkg_descriptor?

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 updated it to pkg_or_key. I think its better because pkg_descriptor does not express well that it still could be a whole package.

@j-mracek
Copy link
Member

Thanks a lot. LGTM

@j-mracek
Copy link
Member

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit 697dcf3 has been approved by j-mracek

@rh-atomic-bot
Copy link

⌛ Testing commit 697dcf3 with merge 2581b92...

rh-atomic-bot pushed a commit that referenced this pull request Oct 22, 2019
Differentiate between pkg that triggered scriptlet and pkg which owns it

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

Closes: #1471
Approved by: j-mracek
@rh-atomic-bot
Copy link

💥 Test timed out

@kontura
Copy link
Contributor Author

kontura commented Oct 24, 2019

@rh-atomic-bot retry

@rh-atomic-bot
Copy link

⌛ Testing commit 697dcf3 with merge 14c0a77...

@rh-atomic-bot
Copy link

☀️ Test successful - status-papr
Approved by: j-mracek
Pushing 14c0a77 to master...

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.

None yet

6 participants