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

Add ems_refresh.openshift.store_unused_images setting #9

Merged
merged 3 commits into from
Aug 29, 2017
Merged

Add ems_refresh.openshift.store_unused_images setting #9

merged 3 commits into from
Aug 29, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented May 3, 2017 •
edited
Loading

openshift part of ManageIQ/manageiq#14662 after repo split.
Depends on ManageIQ/manageiq-providers-kubernetes#11

Strawman alternative/baseline for ManageIQ/manageiq#14628.
Will allow to still get_container_images=true, but instead of saving metadata (labels etc) on all images openshift gave us, save it only for images that have been mentioned by pods.

cc @agrare @enoodle @zeari
Recommend reviewing by commits (refactoring / new tests).

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

This was referenced May 3, 2017
Merged
Closed
@cben
Copy link
Contributor Author

cben commented May 8, 2017

@enoodle @zeari @simon3z Please review.

enoodle
enoodle approved these changes May 8, 2017
View reviewed changes
Copy link

@enoodle enoodle left a comment

Choose a reason for hiding this comment

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

LGTM I think the test will turn green after the PR this is based on will be merged.

@simon3z
Copy link
Contributor

simon3z commented May 9, 2017

@cben can you check travis?

@cben
Copy link
Contributor Author

cben commented May 9, 2017

Travis should pass after ManageIQ/manageiq-providers-kubernetes#11 lands.

@miq-bot
Copy link
Member

miq-bot commented May 9, 2017

This pull request is not mergeable. Please rebase and repush.

@cben cben force-pushed the config_unused_images branch from c2553c0 to fc05dee Compare May 9, 2017 13:07
@cben
Copy link
Contributor Author

cben commented May 9, 2017

rebased. weird, now only hakiri & codeclimate run, travis didn't — close-cycling to have it properly fail :)

@cben cben closed this May 9, 2017
@cben cben reopened this May 9, 2017
@miq-bot
Copy link
Member

miq-bot commented May 11, 2017

This pull request is not mergeable. Please rebase and repush.

@cben cben force-pushed the config_unused_images branch from fc05dee to 7d18874 Compare May 22, 2017 10:08
@miq-bot
Copy link
Member

miq-bot commented May 24, 2017

This pull request is not mergeable. Please rebase and repush.

@cben
Copy link
Contributor Author

cben commented Jul 9, 2017

Closing per ManageIQ/manageiq-providers-kubernetes#11, can be revived if we decide to.

@cben cben closed this Jul 9, 2017
@cben cben mentioned this pull request Jul 20, 2017
Merged
2 tasks
This was referenced Aug 28, 2017
Merged
Closed
@cben cben reopened this Aug 29, 2017
@cben cben force-pushed the config_unused_images branch from 7d18874 to 2bd5bfd Compare August 29, 2017 13:05
@cben
Copy link
Contributor Author

cben commented Aug 29, 2017 •
edited
Loading

@enoodle @moolitayer @Ladas @agrare Please review (recommend by commits).

Graph refresh test fails on child objects (labels etc) getting deleted when this causes an image to be archived. Might affect other scenarios, will investigate and open issue/BZ, for now only need this option in save_inventory mode (the customer that needs this runs euwe).
UPDATE: skipped the test in graph refresh.

@simon3z
Copy link
Contributor

simon3z commented Aug 29, 2017

@agrare @moolitayer can anyone review/approve? Thanks!

@simon3z simon3z requested a review from Ladas August 29, 2017 15:31
@cben cben force-pushed the config_unused_images branch from c64c084 to 1ce0bbb Compare August 29, 2017 15:32
cben added 3 commits August 29, 2017 18:51
@cben
refresh_parser_spec: refactor @data, @data_index access.
c6db7c3
@cben
Add ems_refresh.openshift.store_unused_images setting
1b00d2b 
Allows to still get_container_images=true, but instead of saving
metadata (labels etc) on all images openshift gave us,
save it only for images that have been mentioned by pods.
@cben
Skip store_unused_images deleting metadata test in graph refresh
27f96ed 
Need to investigate, but at this moment only care about backporting
store_unused_images to old refresh.
@cben cben force-pushed the config_unused_images branch from 1ce0bbb to 27f96ed Compare August 29, 2017 15:52
@miq-bot
Copy link
Member

miq-bot commented Aug 29, 2017

Checked commits cben/manageiq-providers-openshift@c6db7c3~...27f96ed with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

agrare
agrare approved these changes Aug 29, 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

@simon3z simon3z merged commit 6a62bb2 into ManageIQ:master Aug 29, 2017
@agrare
Copy link
Member

agrare commented Aug 29, 2017

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

@moolitayer moolitayer added this to the Sprint 68 Ending Sep 4, 2017 milestone Aug 30, 2017
cben pushed a commit to cben/manageiq that referenced this pull request Aug 30, 2017
@simon3z @cben
Backport
162caf9 
ManageIQ/manageiq-providers-kubernetes#11, ManageIQ/manageiq-providers-openshift#9

Option needed for new ems_refresh.openshift.store_unused_images setting

(cherry picked from ManageIQ/manageiq-providers-kubernetes@0046549)

Add ems_refresh.openshift.store_unused_images setting

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

(partially cherry picked from ManageIQ/manageiq-providers-openshift@6a62bb2:
omitted the graph refresh test skip,
passing options explicitly - constructor didn't set `@options` on fine,
adjusted for different test method signature :connected vs :archived)
@cben cben mentioned this pull request Aug 30, 2017
Merged
cben pushed a commit to cben/manageiq that referenced this pull request Aug 31, 2017
@simon3z @cben
Backport
2127843 
ManageIQ/manageiq-providers-kubernetes#11, ManageIQ/manageiq-providers-openshift#9

Option needed for new ems_refresh.openshift.store_unused_images setting

(cherry picked from ManageIQ/manageiq-providers-kubernetes@0046549)

Add ems_refresh.openshift.store_unused_images setting

(partially cherry picked from ManageIQ/manageiq-providers-openshift@6a62bb2:
omitted the graph refresh test skip,
passing options explicitly - constructor didn't set `@options` on euwe,
adjusted for different test method signature :connected vs :archived,
simulating missing stub_settings_merge,
adjusted for different VCR cassette)

https://bugzilla.redhat.com/show_bug.cgi?id=1486483
@cben cben mentioned this pull request Aug 31, 2017
Merged
1 task
@simaishi
Copy link

Backported to Euwe via ManageIQ/manageiq#15917

@simaishi
Copy link

Backported to Fine via ManageIQ/manageiq#15907

d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
@simon3z @d-m-u
Backport
1dc219c 
ManageIQ/manageiq-providers-kubernetes#11, ManageIQ/manageiq-providers-openshift#9

Option needed for new ems_refresh.openshift.store_unused_images setting

(cherry picked from ManageIQ/manageiq-providers-kubernetes@0046549)

Add ems_refresh.openshift.store_unused_images setting

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

(partially cherry picked from ManageIQ/manageiq-providers-openshift@6a62bb2:
omitted the graph refresh test skip,
passing options explicitly - constructor didn't set `@options` on fine,
adjusted for different test method signature :connected vs :archived)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agrare agrare agrare approved these changes

@Ladas Ladas Awaiting requested review from Ladas

@enoodle enoodle Awaiting requested review from enoodle

@moolitayer moolitayer Awaiting requested review from moolitayer

Assignees
No one assigned
Labels
enhancement euwe/backported fine/backported
Projects
None yet
Milestone
No milestone
Development

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