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

[Feedback requested] Dropping redirectors and updating text output for sosreport #3374

Open
TurboTurtle opened this issue Oct 6, 2023 · 5 comments

Comments

@TurboTurtle
Copy link
Member

It's been a few years since we released sos-4.0 and changed the binary to sos, and along with it clarified that the tool is sos, of which report is a function.

I'd like to propose dropping the sosreport and sos-collector redirectors in an upcoming release. I'd ideally say 4.7, but that's probably not feasible since we're in 4.6 currently and most downstreams, I'd assume, want a little more time to coordinate the change.

So how do we feel about 4.7 or 4.8 for no longer carrying these wrappers?

Secondly, along with this change I'd like to update the text output from sosreport to sos (space) report globally. We've got a most of the project using this for a bit now, but the former still in use in the initial banner text, as well as the most-likely-critical "Your sosreport has been generated" message. Note I am not proposing that the tarball name change in any way.

I imagine there is a decent amount of automation looking for that string (in fact, we ourselves look for it during sos collect runs), so this is a change that needs to be well communicated.

I'd like both of these changes to go out at the same time, so if we need to wait until 4.8 for our downstreams to prepare for it, then so be it.

@pmoravec
Copy link
Contributor

pmoravec commented Oct 9, 2023

Cc @jcastill and @mhradile at least.

AFAIK original plans were to keep the shortcut/redirectors for the sos-4.* timeframe. While the planned change itself is sound, my gut feeling is users are not yet prepared, still thinking about "sosreport (tarball) is taken by sosreport (command)". We do have a warning text Please note the 'sosreport' command has been deprecated but it is hard to say how many people read the leading text (or realize its implication). People usually run sosreport, wait for the outcome and copy the generated tarball. So maybe we shall put such string (also) at the end of the stdout where it gets more attention as a heads up, now?

Also, what would be the justification for the redirectors drop - just "we have the legacy redirectors for a while"? Isn't better message "here we add another sos subcommand (e.g. sos upload) under the sos umbrella and it is the good time to drop the redirectors"?

@mhradile
Copy link
Contributor

mhradile commented Oct 9, 2023 via email

@TurboTurtle
Copy link
Member Author

TurboTurtle commented Oct 10, 2023

AFAIK original plans were to keep the shortcut/redirectors for the sos-4.* timeframe.

I mean, we only went to 4.x instead of continuing the 3.x line because of the new binary. I don't see anything on the horizon that would want us to move to a 5.x versioning scheme (also, just going to sidestep the entire versioning scheme debate minefield here). Software evolves and we should be mindful of backwards compatibility for sure, but we shouldn't be shackled to maintaining old entrypoints.

While the planned change itself is sound, my gut feeling is users are not yet prepared, still thinking about "sosreport (tarball) is taken by sosreport (command)".

It's been over 3 years at this point; how much preparation is realistically needed?

Beyond that we should want to encourage an understanding that sos does more than "just" the report function. One of the easiest ways to do this is to have users notice "hey, report is a subcommand, what else can this thing do?"

We do have a warning text Please note the 'sosreport' command has been deprecated but it is hard to say how many people read the leading text (or realize its implication). People usually run sosreport, wait for the outcome and copy the generated tarball. So maybe we shall put such string (also) at the end of the stdout where it gets more attention as a heads up, now?

I'd definitely be open to adding a footer message for a minor version cycle before we drop the redirectors.

Also, what would be the justification for the redirectors drop - just "we have the legacy redirectors for a while"? Isn't better message "here we add another sos subcommand (e.g. sos upload) under the sos umbrella and it is the good time to drop the redirectors"?

As I recall, sos upload was shot down when I brought it up previously in an SST meeting - but less pedantically to your point of tying it to another subcommand being added...it'd be nice but I don't think we should marry the two. By that same token, if there are subcommands you/anyone think would be a useful addition let's get a discussion going on that separately from this one.

Our (old and nowhere near migrated) tests are definitely not ready.

I'm going to be politically nice here and not comment on this point.

@jcastill
Copy link
Member

What about adding the text "Please note the 'sosreport' command has been deprecated" in 4.7, leave it for 4.8 (and perhaps expand it with a 'will be completely dropped in the next minor release', and drop redirectors in 4.9 (or even 4.10 if we consider that there will be 4.xx) ?

We have this documented in many places already, and two minor releases may give us enough time to finish whatever else is needed regarding communication.

Our (old and nowhere near migrated) tests are definitely not ready.

@mhradile we can discuss this, but hopefully two minor releases may give us enough time to plan for the change?

@pmoravec
Copy link
Contributor

I am ok having that explicit statement "sosreport command will be removed in the (next | 4.X) release" around the "sosreport has been generated at" text, and react accordingly :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants