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

Use images to get registry pods using the registry instead of running it. #18148

Merged

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Oct 31, 2018

Description

Currently, number of pods is the number of pods that run the registry-service.
It makes more seance that number of pods will count the number of pods using the registry-service.

This PR changes the meaning of Container Pods from running the registry to using it.

Screenshot
manageiq image registries

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

@yaacov
Copy link
Member Author

yaacov commented Oct 31, 2018

@cben @Ladas @agrare please review

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.

👍

@agrare
Copy link
Member

agrare commented Oct 31, 2018

@yaacov IDK about changing the meaning of an existing field, though I agree I would have expected this to be the number of pods using not running the registry.

Can we add another association for the old meaning?

has_many :service_container_groups, :through => :container_services, :as => :container_group or something? Not sure what best to call it.

@yaacov
Copy link
Member Author

yaacov commented Oct 31, 2018

@agrare @cben please re-review

@cben
Copy link
Contributor

cben commented Oct 31, 2018

container_groups should definitely match containers, so 👍 to this change.

I don't think number of pods serving the registry would be useful for anyone, only increasing confusing (if one really needs, click the services). But adding an association is harmless as long as we don't show it :-)

I'd be in favor of renaming container_services to a better name, at least in the UI. But not in this PR.

@agrare
Copy link
Member

agrare commented Oct 31, 2018

I don't think number of pods serving the registry would be useful for anyone

Yeah this might very well be true, I just don't want someone to have e.g. built a report and now they can't get at the data at all.

@yaacov yaacov force-pushed the get-pods-from-images-instead-of-services branch from 535772f to 56d5ff6 Compare October 31, 2018 13:02
@yaacov
Copy link
Member Author

yaacov commented Oct 31, 2018

@agrare @cben update the inline docs, please re-review

@JPrause
Copy link
Member

JPrause commented Oct 31, 2018

@miq-bot add_label blocker

@agrare
Copy link
Member

agrare commented Oct 31, 2018

Blocker? @yaacov this isn't a regression right?

@yaacov
Copy link
Member Author

yaacov commented Oct 31, 2018

@agrare

Blocker?

the BZ is blocker: https://bugzilla.redhat.com/show_bug.cgi?id=1599696

@yaacov this isn't a regression right?

Yes not a regression, Dave Johnson asked it to be registered as one here: https://bugzilla.redhat.com/show_bug.cgi?id=1599696#c2

@agrare
Copy link
Member

agrare commented Oct 31, 2018

Okay thanks @yaacov I'll comment on the BZ.

@agrare agrare merged commit 22f6e09 into ManageIQ:master Oct 31, 2018
@agrare agrare added this to the Sprint 98 Ending Nov 5, 2018 milestone Oct 31, 2018
@simaishi
Copy link
Contributor

simaishi commented Nov 1, 2018

@yaacov @agrare hammer/yes ?

@agrare
Copy link
Member

agrare commented Nov 1, 2018

@simaishi updated

simaishi pushed a commit that referenced this pull request Nov 2, 2018
…-services

Use images to get registry pods using the registry instead of running it.

(cherry picked from commit 22f6e09)

https://bugzilla.redhat.com/show_bug.cgi?id=1599696
@simaishi
Copy link
Contributor

simaishi commented Nov 2, 2018

Hammer backport details:

$ git log -1
commit 707c21e3477e06ab39b6ee420633bf9de9c2b4ef
Author: Adam Grare <agrare@redhat.com>
Date:   Wed Oct 31 10:31:58 2018 -0400

    Merge pull request #18148 from yaacov/get-pods-from-images-instead-of-services
    
    Use images to get registry pods using the registry instead of running it.
    
    (cherry picked from commit 22f6e0974bd4960b4c40a4fc53a5403b48a174a0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1599696

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.

7 participants