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

Check if updates weren't installed to not hard fail #56256

Merged
merged 15 commits into from
May 18, 2020

Conversation

xeacott
Copy link
Contributor

@xeacott xeacott commented Feb 26, 2020

What does this PR do?

Corrects the list of updates that were actually installed by the WUA and returns a list of updates that
were not installed on the box due to various reasons, superseded, cumulative update, etc.

What issues does this PR fix or reference?

#52387

Tests written?

Needs tests

Commits signed with GPG?

No

Unverified

This user has not yet uploaded their public signing key.
@xeacott xeacott requested a review from a team as a code owner February 26, 2020 19:42
@ghost ghost requested a review from DmitryKuzmenko February 26, 2020 19:42

Unverified

This user has not yet uploaded their public signing key.
twangboy
twangboy previously approved these changes Feb 26, 2020

Unverified

This user has not yet uploaded their public signing key.
@DmitryKuzmenko
Copy link
Contributor

@xeacott last change was made 3 weeks ago. Is it still WIP?

Unverified

This user has not yet uploaded their public signing key.
@DmitryKuzmenko DmitryKuzmenko self-assigned this Apr 3, 2020
@DmitryKuzmenko DmitryKuzmenko added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Apr 3, 2020
DmitryKuzmenko
DmitryKuzmenko previously approved these changes Apr 3, 2020
Copy link
Contributor

@DmitryKuzmenko DmitryKuzmenko left a comment

Choose a reason for hiding this comment

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

Approving in assumption there would be a test.

@DmitryKuzmenko
Copy link
Contributor

@xeacott any updates on this?

@xeacott
Copy link
Contributor Author

xeacott commented Apr 14, 2020

Yes sorry, have not been able to focus on Open PRs for quite some time now - anyways, this needs an entire test suite written for the module and I haven't had a chance yet ... but I'll add that to my list for this week.

@xeacott
Copy link
Contributor Author

xeacott commented Apr 15, 2020

There exists this PR #56637 that includes additions to unit and integration tests that I will utilize in this PR, so I will wait for that PR to be merged and then tests will be written.

Unverified

This user has not yet uploaded their public signing key.
@xeacott xeacott dismissed stale reviews from DmitryKuzmenko and twangboy via 8561132 April 15, 2020 18:58
@xeacott xeacott changed the title [WIP] Check if updates weren't installed to not hard fail Check if updates weren't installed to not hard fail May 14, 2020
@xeacott xeacott removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label May 15, 2020
Copy link
Contributor

@DmitryKuzmenko DmitryKuzmenko left a comment

Choose a reason for hiding this comment

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

Just 2 minor performance related change requests.

Comment on lines 490 to 495
updates_not_installed = [item for item in install.list() if item not in post_info]

# Verify the installation
for item in install.list():
if not salt.utils.data.is_true(post_info[item]["Installed"]):
ret["changes"]["failed"] = {
item: {
"Title": post_info[item]["Title"][:40] + "...",
"KBs": post_info[item]["KBs"],
# Only check for items that were installed
if item not in updates_not_installed:
Copy link
Contributor

Choose a reason for hiding this comment

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

These two loops are iterating the same list so could be one:

updates_not_installed = []
for item in install.list():
    if item not in post_info:
        updates_not_installd.append(item)
    else:

the next line is the if below.


# Add the list of updates not installed to the return
if len(updates_not_installed) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder lint said you nothing about his. Please change to if updates_not_installed:

@dwoz dwoz merged commit 0b36082 into saltstack:master May 18, 2020
@sagetherage sagetherage added the ZRelease-Sodium retired label label May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZRelease-Sodium retired label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants