Display "Published ports" information when output format is JSON #887
Conversation
steiler
force-pushed
the
mysocketiojsonoutput
branch
2 times, most recently
from
5f81584
to
7fe1f8a
Compare
|
@hellt: Tested and ready for review now. |
|
Hi @steiler |
steiler
force-pushed
the
mysocketiojsonoutput
branch
from
381a3c2
to
5443568
Compare
|
Alright, all green! |
| @@ -35,6 +35,10 @@ type TopoFile struct { | |||
| name string // file name without extension | |||
| } | |||
|
|
|||
| func (tf *TopoFile) GetDir() string { | |||
There was a problem hiding this comment.
just curious why you wanted this function, instead of accessing the field?
There was a problem hiding this comment.
the issue is, that we put applogic in the cmd package ... what we should do is put all the app logic in the clab package and just do parameter parsing and population in the cmd package... that is not too clean of a structure. As a result of this, from the cmd package we are unable to access the lower case dir variable... instead of opting for making the var public (CamelCase) I introduced this getter.
We could come up with a seperate PR and seperate app logic from parameter paring and initialization if you like, I'd be happy to do so.
cmd/inspect.go
Outdated
| if len(containers) == 0 { | ||
| log.Println("no containers found") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
we need to find a way of not printing an empty table if no containers are found
right now it prints an empty one, and before we used "no containers found"
There was a problem hiding this comment.
The issue was, that this was printed even in the json case ... which is bad when json output is expected.
What exactly is the issue with the empty table?
There was a problem hiding this comment.
it just doesn't make a lot of sense to me printing an empty table. I haven't seen any other tool doing that. Gives an impression that something
we need to make sure it is not printed when format is set to json and printed when default output. I can have a look at it if you have more interesting things to have a crack on
There was a problem hiding this comment.
I'd personally prefere a stable output method. But nevermind.
Check out the new commit.
bcc6a2d
There was a problem hiding this comment.
why revert the test based on json?
lets keep both?! Just to make sure json output is also fine in the zero container case.
cmd/inspect.go
Outdated
| // mySocketIoTokenFileFromBindMounts finds a node of kind mysocketio. | ||
| // if that is found, the bindmounts are searched for ".mysocketio_token" and the path is being converted into an | ||
| // absolute path and returned. | ||
| func mySocketIoTokenFileFromBindMounts(_nodes map[string]nodes.Node, configPath string) (string, error) { |
There was a problem hiding this comment.
Hi @steiler
there is one little thing with this approach which I am ok to let slip
when you do clab ins -a this func will not work since we don't have c.Nodes populated as we don't read topology files.
One workaround for that is to use GenericContainer type where i added mounts information in this commit. We can rework this function to work on a slice of container info, rather than c.Nodes, and get the binds info from it. But then the problem is what to do if you have >1 mysocketio node, with different tokens...
So for the time being I just made a shunt so that mysocketdata is not fetched if we use inspect --all
There was a problem hiding this comment.
oh ok, I see.
What if we then simply print multiple tables indexed with either the containername or maybe better with the lab name, availabel via the container labels?!
In the json case, that would require a change to the output format as well ... so we eiter make the highlevel mysocketio entry a simple list, also carrying the labname as on item of the enclosed entries or a map with the labname as index...
Thought?
There was a problem hiding this comment.
yes, I think adding lab name as a field for mysocket data is a valid approach. I would go with a list where lab name is a k:v pair inside the list
when inspect --all is used and the output is in the table format, then I think it is safe to have a single table that lists set up sockets with a field listing a lab for which these sockets were created.
we would just need to make sure we list through all containers which have label[kind]=mysocketio, get their respective source mount path and fetch the sockets.
Quite some work. Maybe better to do that in a separate PR?
|
I think we have discussed this before and wrongfully asked to put info message back when no containers found. |
|
Alright, @hellt, took me quite a while to get back to this, but take a look at the actual changes. |
|
Thanks @steiler ! |
This PR is to implement what is mentioned in Issue #634.
The actual implementation is a breaking change, since the up until now top level array of container data moved under the "container_data" entry and at the same time introduces another array called mysocket_data which keeps the mysocketio information.
This still needs to be tested. Actually it seems like my endpoint protection considers mysocketio harmful and blocks it.