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.

Sign up for GitHub

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

Jump to bottom

Changes needed for openshift graph refresh #57

Merged
merged 9 commits into from
Jul 21, 2017
Merged

Changes needed for openshift graph refresh #57

merged 9 commits into from
Jul 21, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented Jul 12, 2017 •
edited
Loading

Initializing the openshift inv_collections arguably belongs in openshift
repo, but not bothering with move as @agrare has plans to move them both
to core repo.

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

@miq-bot miq-bot added the wip label Jul 12, 2017
@cben cben mentioned this pull request Jul 12, 2017
Merged
2 tasks
@cben cben force-pushed the graph-openshift branch 2 times, most recently from 946bb5f to f268f30 Compare July 13, 2017 12:40
@cben cben changed the title [WIP] Changes needed for openshift graph refresh Changes needed for openshift graph refresh Jul 13, 2017
@cben
Copy link
Contributor Author

cben commented Jul 13, 2017

Yay, ManageIQ/manageiq-providers-openshift#34 now passes all tests with this :-)
cc @agrare @Ladas @simon3z

@miq-bot miq-bot removed the wip label Jul 13, 2017
@cben cben mentioned this pull request Jul 13, 2017
Closed
24 tasks
Ladas
Ladas approved these changes Jul 14, 2017
View reviewed changes
Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

Awesome, it works for me. 👍

I had to do few minor fixes, that I've seen because I have DB unique constraints. But we can add those as followups.

My WIP is here #64, need to clean it up

@cben cben force-pushed the graph-openshift branch from f268f30 to a062ecd Compare July 16, 2017 14:17
@cben cben mentioned this pull request Jul 16, 2017
Closed
@cben cben force-pushed the graph-openshift branch from a062ecd to 15fd322 Compare July 16, 2017 14:27
simon3z
simon3z reviewed Jul 17, 2017
View reviewed changes
app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb
@@ -277,7 +285,7 @@ def get_namespaces_graph(inv)

container_project = collection.build(h)

get_custom_attributes_graph(container_project, custom_attrs)
get_custom_attributes_graph(container_project, custom_attrs) # TODO: untested
Copy link
Contributor

Choose a reason for hiding this comment

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

@cben is this comment still relevant? When do you think this will be tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still relevant, none of our VCR recordings have project labels. (but I'm confident this works)
I didn't bother Erez with adding those while he re-recorded because there are a few more things we have 0 of, I'm still collecting the list...

@cben cben force-pushed the graph-openshift branch from 15fd322 to 2ebd841 Compare July 17, 2017 16:25
agrare
agrare approved these changes Jul 18, 2017
View reviewed changes
Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

LGTM

@cben cben force-pushed the graph-openshift branch from 7ecb614 to 6c24d7d Compare July 19, 2017 14:07
@cben
Copy link
Contributor Author

cben commented Jul 19, 2017 •
edited
Loading

Implemented archiving using :delete_method => :disconnect_inv.
Refactored the whole refresher_spec file to be run twice:

> rspec .../refresher_spec.rb -f documentation

ManageIQ::Providers::Kubernetes::ContainerManager::Refresher
  graph refresh
    .ems_type
    will perform a full refresh on k8s
    when refreshing an empty DB
      saves the objects in the DB
      when refreshing non empty DB
        removes the deleted objects from the DB
        disconnects objects
        archives objects
  classical refresh
    will perform a full refresh on k8s
    .ems_type
    when refreshing an empty DB
      saves the objects in the DB
      when refreshing non empty DB
        removes the deleted objects from the DB
        archives objects
        disconnects objects

@zakiva can you review, especially the specs refactor?
I'll do the same in ManageIQ/manageiq-providers-openshift#34

zakiva
zakiva approved these changes Jul 19, 2017
View reviewed changes
Copy link
Contributor

@zakiva zakiva left a comment

Choose a reason for hiding this comment

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

@cben spec refactor looks very good!

Ladas
Ladas approved these changes Jul 20, 2017
View reviewed changes
Copy link
Contributor

@Ladas Ladas left a comment

Choose a reason for hiding this comment

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

The new changes look great

@cben
Leave images labels & docker_labels collections up to openshift
c15d51f 
Openshift refresh_parser has a setting where it won't fetch image
metadata but should not remove previously set metadata,
which requires not including these InventoryCollections.
@cben cben force-pushed the graph-openshift branch from d09dca9 to c15d51f Compare July 20, 2017 13:40
@miq-bot
Copy link
Member

miq-bot commented Jul 20, 2017

Checked commits cben/manageiq-providers-kubernetes@cb01002~...c15d51f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. 🍰

@cben
Copy link
Contributor Author

cben commented Jul 20, 2017

I had to take out image labels & docker_labels collections, will init them conditionally in openshift PR.

@moolitayer, @simon3z This is ready.

@simon3z simon3z merged commit 005b63c into ManageIQ:master Jul 21, 2017
@moolitayer moolitayer added this to the Sprint 65 Ending Jul 24, 2017 milestone Aug 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simon3z simon3z simon3z left review comments

@agrare agrare agrare approved these changes

@zakiva zakiva zakiva approved these changes

@Ladas Ladas Ladas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@cben @miq-bot @simon3z @Ladas @zakiva @agrare @moolitayer