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

Save inventory container: remove target option #15182

Merged
merged 1 commit into from Jul 11, 2017
Merged

Conversation

@zakiva
Copy link
Contributor

zakiva commented May 22, 2017

This logic was probably meant to support a targeted refresh which we do not use or plan to. However, it actually causes us a problem since the target isn't being passed through the recursive calls.
Partial refresh for containers entities will be introduced as part of the container graph refresh in #14337. Meanwhile, I suggest we remove this code:

  • Remove target argument from save inventory container methods.
  • Remove deletes variable and always pass :use_association instead.

This should solve:
https://bugzilla.redhat.com/show_bug.cgi?id=1436132
https://bugzilla.redhat.com/show_bug.cgi?id=1451832

Still need to add tests.

cc @cben @moolitayer @simon3z

@miq-bot miq-bot added the wip label May 22, 2017
@zakiva

This comment has been minimized.

Copy link
Contributor Author

zakiva commented May 22, 2017

@miq-bot add_label providers/containers, bug

@simon3z

This comment has been minimized.

Copy link
Contributor

simon3z commented May 22, 2017

cc @cben @moolitayer
This worries me a little because I have the feeling we'll need some really good testing before deeming it stable enough for a stable release.

@cben

This comment has been minimized.

Copy link
Contributor

cben commented May 23, 2017

We didn't do really good testing when introduced this broken code several stable releases ago ;-)
I'm very interested in more tests, we should assume graph refresh will break any untested aspects, with an emphasis on deletions.

Current tests I'm aware of check only:

  • Refresh into empty DB.
  • Idempotence: 2nd refresh of unchanged inventory into same DB.

Don't have but ought to:

  • Additions/changes then refresh. (Not for this PR, but we want some.)
  • Deletions then refresh. Should also test what gets deleted vs disconnected.

Had some discussions with @zakiva, with some ideas how to reproducibly record more complex scenarios.

@moolitayer

This comment has been minimized.

Copy link
Contributor

moolitayer commented May 24, 2017

Would love to see this unused code go away

@zakiva zakiva changed the title [WIP] Save inventory container: remove target option Save inventory container: remove target option Jun 1, 2017
@miq-bot miq-bot removed the wip label Jun 1, 2017
@simon3z

This comment has been minimized.

Copy link
Contributor

simon3z commented Jun 1, 2017

@cben @moolitayer can you test/review/approve?
How confident are we on this fix? Have you tested/verified this properly?

@cben
cben approved these changes Jun 20, 2017
Copy link
Contributor

cben left a comment

@zakiva are the tests ManageIQ/manageiq-providers-openshift#18 close to ready? Can I help?

I'm confident in this to 👍 merging before the tests (and you have been testing this a lot), but I think it's good procedure to have tests before backporting.

@zakiva

This comment has been minimized.

Copy link
Contributor Author

zakiva commented Jun 21, 2017

@zakiva are the tests ManageIQ/manageiq-providers-openshift#18 close to ready?

Yes, added also the k8s tests in ManageIQ/manageiq-providers-kubernetes#44 both pending review/merge

@cben
cben approved these changes Jun 25, 2017
Copy link
Contributor

cben left a comment

kubernetes tests merged, ManageIQ/manageiq-providers-openshift#18 is only pending on this PR, right?

app/models/ems_refresh/save_inventory_container.rb Outdated
@@ -1,6 +1,5 @@
module EmsRefresh::SaveInventoryContainer
def save_ems_container_inventory(ems, hashes, target = nil)

This comment has been minimized.

Copy link
@moolitayer

moolitayer Jun 25, 2017

Contributor

_target

@zakiva

This comment has been minimized.

Copy link
Contributor Author

zakiva commented Jun 25, 2017

kubernetes tests merged, ManageIQ/manageiq-providers-openshift#18 is only pending on this PR, right?

@cben yes

@zakiva zakiva force-pushed the zakiva:remove_target branch to c2ce88f Jun 25, 2017
@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Jun 25, 2017

Checked commit zakiva@c2ce88f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

Copy link
Contributor

moolitayer left a comment

LGTM 👍
Hurry for removing unused code!

@moolitayer

This comment has been minimized.

Copy link
Contributor

moolitayer commented Jun 25, 2017

@simon3z please review

@simon3z

This comment has been minimized.

Copy link
Contributor

simon3z commented Jul 11, 2017

@miq-bot add_label fine/yes

@miq-bot miq-bot added the fine/yes label Jul 11, 2017
Copy link
Contributor

simon3z left a comment

LGTM 👍
cc @chrispy1

@blomquisg

This comment has been minimized.

Copy link
Member

blomquisg commented Jul 11, 2017

Overall this looks OK to me, but I've asked @agrare to be the merger on it to make sure I didn't miss anything. A change like this has a chance of nasty side effects if something fundamental is missed, and Adam has been living in Inventory for several months now.

@agrare
agrare approved these changes Jul 11, 2017
Copy link
Member

agrare left a comment

You're right save_inventory_multi doesn't have a way to pass args to save_child_inventory None of the other child save methods took a target parameter but I see at least save_labels_inventory here does.

👍 to this since the target was never used here anyway. In the future we could add the ability for save_inventory_mutli to pass other args to the child save methods

@agrare agrare merged commit aa7b13f into ManageIQ:master Jul 11, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.1%) to 73.069%
Details
@simon3z

This comment has been minimized.

Copy link
Contributor

simon3z commented Jul 11, 2017

@agrare @blomquisg we have the FINE backport here:

#15543

It also includes extra-tests to be on the safe side.

@simon3z

This comment has been minimized.

Copy link
Contributor

simon3z commented Jul 11, 2017

@agrare @blomquisg we have the FINE backport here:

#15543

It also includes extra-tests to be on the safe side.

@agrare @blomquisg I asked @cben and @zakiva to have a FINE PR with both the extra-tests and this PR, although it doesn't seem that one yet (it seems it's just the tests).

@agrare

This comment has been minimized.

Copy link
Member

agrare commented Jul 11, 2017

@zakiva can you cherry-pick the merge commit to your fine pr with the added tests? That way the tests can run on the PR before backporting it.

@blomquisg

This comment has been minimized.

Copy link
Member

blomquisg commented Jul 11, 2017

@simon3z 😄 I was a little confused looking at #15543.

If you want to have #15543 be the Fine backport for this PR, just remember to remove the Fine/yes label from this PR.

@zakiva

This comment has been minimized.

Copy link
Contributor Author

zakiva commented Jul 11, 2017

@miq-bot rm_label fine/yes
will backport in #15543 with some tests

@simaishi

This comment has been minimized.

Copy link
Contributor

simaishi commented Jul 11, 2017

Backported to Fine via #15543

@simaishi

This comment has been minimized.

Copy link
Contributor

simaishi commented Jul 14, 2017

@zakiva @agrare The BZ flag indicates this change needs to be backported to Euwe branch. Not sure if #15543 should be backported to Euwe or you'd need a separate PR for Euwe. Please advise.

@zakiva

This comment has been minimized.

Copy link
Contributor Author

zakiva commented Jul 16, 2017

Sent a separate PR for Euwe: #15573

@simaishi

This comment has been minimized.

Copy link
Contributor

simaishi commented Jul 18, 2017

Backported to Euwe via #15573

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.