Skip to content

refactor info#5507

Merged
openshift-merge-robot merged 1 commit into
containers:masterfrom
baude:newinfo
Apr 6, 2020
Merged

refactor info#5507
openshift-merge-robot merged 1 commit into
containers:masterfrom
baude:newinfo

Conversation

@baude
Copy link
Copy Markdown
Member

@baude baude commented Mar 15, 2020

the current implementation of info, while typed, is very loosely done so. we need stronger types for our apiv2 implmentation and bindings.

Signed-off-by: Brent Baude bbaude@redhat.com

@baude baude requested a review from mheon March 15, 2020 16:55
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 15, 2020
@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 15, 2020
@baude
Copy link
Copy Markdown
Member Author

baude commented Mar 15, 2020

This PR will not build nor pass any tests.

@baude
Copy link
Copy Markdown
Member Author

baude commented Mar 15, 2020

@mheon I know we have talked about this for a while ... the current state of info will soon block finalization of the apiv2. I'm only looking for general confirmation here of what you think. An actual review can come once I finish this PR. If you don't like the approach, it makes no sense to go further.

Copy link
Copy Markdown
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

General approach LGTM!

I wonder if it would make sense to create a pkg/info for it which would provide the types and the code (bonus points for unit tests). @mheon suggested doing that for the auto-update PR and I found it a nice way to debloat ./libpod.

@mheon
Copy link
Copy Markdown
Member

mheon commented Mar 16, 2020

I'm OK with leaving it in Libpod; it seems like a reasonable API endpoint for us.

Overall approach LGTM here.

@baude baude force-pushed the newinfo branch 2 times, most recently from 7ebd433 to 1f51f4b Compare March 17, 2020 19:41
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2020
@baude baude changed the title [WIP]refactor info refactor info Mar 17, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 17, 2020
@edsantiago
Copy link
Copy Markdown
Member

I'm not really keen on the idea of breaking backward compatibility, but I missed the discussion yesterday. If it's absolutely necessary, here is a patch to get system tests passing. I believe there is still a real problem with the info command, though:

$ ./bin/podman info --format '{{.host.arch}}'
Error: template: image:1:7: executing "image" at <.host.arch>: can't evaluate field host in type main.infoWithExtra```
$ /usr/bin/podman info --format '{{.host.arch}}'
amd64

Fails for anything (AFAICT) inside {{.anything}}

@baude baude force-pushed the newinfo branch 4 times, most recently from cf882b5 to bd4b634 Compare March 18, 2020 21:09
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2020
Comment thread libpod/define/info.go Outdated
the current implementation of info, while typed, is very loosely done so.  we need stronger types for our apiv2 implmentation and bindings.

Signed-off-by: Brent Baude <bbaude@redhat.com>
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2020
@mheon
Copy link
Copy Markdown
Member

mheon commented Apr 6, 2020

/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 6, 2020
@baude
Copy link
Copy Markdown
Member Author

baude commented Apr 6, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2020
@openshift-merge-robot openshift-merge-robot merged commit 7d95e0c into containers:master Apr 6, 2020
@baude baude deleted the newinfo branch May 7, 2020 13:11
@github-actions github-actions Bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants