retis-org / retis

Tracing packets in the Linux networking stack & friends
https://retis.readthedocs.io/en/stable/
100 stars 14 forks source link

retis_in_container.sh: let retis fetch custom profiles #298

Closed vlrpl closed 12 months ago

atenart commented 12 months ago

Up until now the retis_in_container script does not need any argument to run, as it's designed to embed the logic to automatically make the right calls. The only example is RETIS_TAG which overrides a built-in value.

I'd like this to work in a similar way here, so most users only have to run the script as-is. A few options here:

Also, I think we should add support for providing profiles as a path, and this would make sense here too as we already map the current directory (for the log file initially). People could do ./retis_in_container.sh -p ./foo.yaml collect.

vlrpl commented 12 months ago

Up until now the retis_in_container script does not need any argument to run, as it's designed to embed the logic to automatically make the right calls. The only example is RETIS_TAG which overrides a built-in value.

I'd like this to work in a similar way here, so most users only have to run the script as-is. A few options here:

* Look for `$HOME/.config/retis/profiles` and map it if found.

* Optionally allow to override that path, or to add an additional one. Not sure if needed.

Not a fan of hidden automatic behaviors. What if one has local profiles but want to use shared profiles from the test team e.g. in a container? It should pollute its directory if the override is not allowed.

Edit: mapping $HOME/.config/retis/profiles or overriding can be fine, maybe redundant (simply you can do the same with $RETIS_PROFILES)

Can you elaborate the reason you're not sure and how overriding could be a problem/bad thing?

Also, I think we should add support for providing profiles as a path, and this would make sense here too as we already map the current directory (for the log file initially). People could do ./retis_in_container.sh -p ./foo.yaml collect.

I'm ok with this last if this is an addition (as it seems you intend). As a replacement it's not the same (only one file mapped) and probably too constraining. Edit: of course, I assume we both consider this out of the scope of this patch

atenart commented 12 months ago

Not a fan of hidden automatic behaviors. What if one has local profiles but want to use shared profiles from the test team e.g. in a container? It should pollute its directory if the override is not allowed.

I wouldn't say this is hidden: it maps to the current behavior and retis_in_container aims at working as close as possible to a plain Retis binary. Also I think it 100% maps to our goal of using sane defaults to improve ux. Then and only then, if there is a need to manually set the value, we can always provide a way, which is the comment below,

Can you elaborate the reason you're not sure and how overriding could be a problem/bad thing?

Not saying it would lead to bad things, more a comment on adding features only if there are users. If you think that is needed, I'm OK with that.

Also, I think we should add support for providing profiles as a path, and this would make sense here too as we already map the current directory (for the log file initially). People could do ./retis_in_container.sh -p ./foo.yaml collect.

I'm ok with this last if this is an addition (as it seems you intend). As a replacement it's not the same (only one file mapped) and probably too constraining.

Yes, as an addition.

vlrpl commented 12 months ago

Not a fan of hidden automatic behaviors. What if one has local profiles but want to use shared profiles from the test team e.g. in a container? It should pollute its directory if the override is not allowed.

I wouldn't say this is hidden: it maps to the current behavior and retis_in_container aims at working as close as possible to a plain Retis binary. Also I think it 100% maps to our goal of using sane defaults to improve ux. Then and only then, if there is a need to manually set the value, we can always provide a way, which is the comment below,

you're right, hidden doesn't fit well.

Can you elaborate the reason you're not sure and how overriding could be a problem/bad thing?

Not saying it would lead to bad things, more a comment on adding features only if there are users. If you think that is needed, I'm OK with that.

yeah, probably not fitting wording as well here. With "bad things" I meant something I could have missed. I edited my previous comment (you were faster in replying :) saying that both are fine (but I kind of think it is too much).

In my case, I needed a profile to create and throw away. Probably the '-p ./path/to/the/profile' would have been useful for me, but without that an override was more convenient (of course, the $HOME approach also would have worked).

Let's go with $HOME, it is trivial to add the override (like for $KCONF) if needed.

Also, I think we should add support for providing profiles as a path, and this would make sense here too as we already map the current directory (for the log file initially). People could do ./retis_in_container.sh -p ./foo.yaml collect.

I'm ok with this last if this is an addition (as it seems you intend). As a replacement it's not the same (only one file mapped) and probably too constraining.

Yes, as an addition.