-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
platformmanagement_public_425 - image usage report tool #9587
Conversation
I'm kind of averse to both of these - they don't fit our existing CLI model and I'm not sure I understand the use case they are solving. Link to trello card here please? |
Ok, that helps. I'd like to refocus this on two things:
I think the underlying stuff you're doing is a good start, but I'd ask a
The output in this PR to start is likely not scriptable, but I expect end I'd kind of expect the following:
The first 3 allow me to perform image related queries. The last one allows I would expect the UI on this to be a tabular representation of any of On Mon, Jun 27, 2016 at 4:31 PM, Maciej Szulik [email protected]
|
See kubernetes/kubernetes#11382 for a rough equivalent. They turn the core data into an intermediate form which is usable. Then they do a simple display with Get. we want the same thing for this. |
This might just be |
Thanks, will check this out. |
@smarterclayton ptal if this is going in the right direction. What is left atm is only the printing logic. The data aggregation part is ready for review. |
@smarterclayton this is ready for review, for now only one possible output is supported but this can be easily expanded as a followup. Please let me know what you think. Here's the sample output:
|
Added tests per @mfojtik request. |
Looks good, two requests:
|
@smarterclayton commands split (1 - checked), for no 2 it doesn't make any sense to add more tests to test-cmd since it doesn't have any images nor imagestreams. I'll work on unittest and maybe some extended as a followup if you don't mind. |
You can create images directly via oc create and then test it. So test-cmd On Tue, Jul 26, 2016 at 8:20 AM, Maciej Szulik [email protected]
|
@smarterclayton added unit tests and cmd tests, ptal. |
os::cmd::expect_success "oc new-project pruner" | ||
os::cmd::expect_success "oc import-image busybox:latest --confirm -n pruner" | ||
os::cmd::expect_success_and_text "oadm top images" "pruner/busybox:latest" | ||
os::cmd::expect_success_and_text "oadm top imagestreams" "pruner/busybox" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to have more aggressive checks here, since none of this information is stable enough. More aggressive checks are done in unit testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just import a fake image that is stable. We have several of those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm... I must have missed those. I'll update.
[test] |
Test flake #8427. [merge] |
Flake #9938 re-[merge] |
Flake #10002. re-[merge] |
#9775 again... re-[merge] |
Fixed the tests comparing array since they were flaking. |
Tightened infos array comparisons more. |
#9775 again... re-[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7417/) (Image: devenv-rhel7_4731) |
Rebased on top of master to hopefully bypass the flake. |
Evaluated for origin test up to 505b462 |
Evaluated for origin merge up to 505b462 |
continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7250/) |
re[merge] |
#9775 again... re-[merge] |
@soltysh @smarterclayton how are we going to handle this in combination with |
@fabianofranz you probably want to chase the upstreams too. |
@deads2k the commands is created in such a way to be possible to hook the subcommands into upstream's |
That's different than the construction of the upstream @fabianofranz how would you try to integrate these two things together? |
We made arguments before about not mixing args and subcommands. Unless |
image|imagestreams where resource arguments, but on Clayton's request I've moved those to subcommands. I don't see any problems changing this later on, as long as we're able to maintain backwards compatibility. Other option is to make these commands experimental, which is good alternative for now. |
Sounds like that should be pushed on upstream. |
This is last element of my card. Sample output looks like this:
@smarterclayton since you've mentioned you don't like #9542 being a separate command. How about joining the two into
oadm images
which can do both based on flags?@miminar first commit is the reuse we've talked about :)