[policy] Re-add logic to request case-id if not present by jcastill · Pull Request #4020 · sosreport/sos

jcastill · 2025-05-21T09:23:48Z

This commit re-adds the logic to ask for case-id if the user hasn't specified it in the command line
explicitly.

Related: RHEL-92071

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

Is the commit message split over multiple lines and hard-wrapped at 72 characters?
Is the subject and message clear and concise?
Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
Does the commit contain a Signed-off-by: First Lastname email@example.com?
Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
Are all passwords or private data gathered by this PR obfuscated?

packit-as-a-service · 2025-05-21T09:27:46Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/sosreport-sos-4020
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

pmoravec · 2025-05-21T11:30:33Z

+        # set or query for case id
+        if not cmdline_opts.batch and not \
+                cmdline_opts.quiet:
+            if caseid:
+                self.commons['cmdlineopts'].case_id = caseid
+            else:
+                self.commons['cmdlineopts'].case_id = input(
+                    _("Optionally, please enter the case id that you are "
+                      "generating this report for [%s]: ") % caseid
+                )


We have the same code in prompt_for_case_id method in https://github.com/sosreport/sos/blob/main/sos/upload/targets/redhat.py#L55-L72 . Shouldnt we have just one routine for that identical code block?

(wont this code block here make the prompt_for_case_id method redundant?)

pmoravec · 2025-05-21T11:33:26Z

Interesting; somewhere between sos-4.8.2 and sos-4.9.0, we stopped asking for the case ID.

Is it worth having a test for this? That would need expect or something similar to interact with non-batched sos report, which can be tricky. So I am not sure..

TurboTurtle · 2025-05-21T12:46:21Z

Interesting; somewhere between sos-4.8.2 and sos-4.9.0, we stopped asking for the case ID.

It got dropped in d75cef3#diff-e7b68e1e530790c6fe03b3482d895db01641b530a16acb9945bbb15d22f7023bL362

Because it was removed from the base distro class and moved solely to the upload component. This should live in distro/__init__.py and be removed from upload.

Is it worth having a test for this? That would need expect or something similar to interact with non-batched sos report, which can be tricky. So I am not sure..

That would be a bit involved, though yes it would be best. I don't know off the top of my head if the existing test classes directly support this, but I'd wager not.

TurboTurtle

As @pmoravec mentioned, this is a duplication of the code from prompt_for_case_id(), which was moved into upload from the base distro/__init__.py.

We shouldn't just copy code into multiple locations. The method needs to be removed from upload, and added back into the base policy class (as a whole method) so that case id is again prompted for across all relevant subcommands, not just upload.

TurboTurtle · 2025-05-21T12:47:41Z

+                    _("Optionally, please enter the case id that you are "
+                      "generating this report for [%s]: ") % caseid


Needs to be an f-string.

pmoravec · 2025-05-28T07:43:37Z

Hi @TurboTurtle and @arif-ali , could you please review this PR? It fixes a regression we would like to have closed.

arif-ali · 2025-05-29T08:20:31Z

I'll have a look today, I've been in and out, and haven't had a chance to look.

arif-ali

looks ok from me, other than a comment below

arif-ali · 2025-05-29T14:30:35Z

                return self.RH_SFTP_HOST
-            if not self.commons['cmdlineopts'].case_id and not\
-               self.prompt_for_case_id():
-                return self.RH_SFTP_HOST


I am presuming this was removed on purpose and you don't need this if a case id is not there?

Also not sure why this is being removed.

Indeed, quite basic use case sos report -o host --batch --upload diverts: it tries to upload to the customer portal while it should use SFTP (no case id).

Instead of 3 lines removal, there must be a change on 2nd line to:

self.commons['policy'].prompt_for_case_id(self.commons['cmdlineopts']):

I had to overview this removal, sorry for ACKing it.

The reason why I removed it was to run some tests and then I pushed the commit without re-adding it, sorry.
Thank you @arif-ali ++ for spotting this :)
I've just pushed a new commit with the code back

TurboTurtle · 2025-05-29T14:41:14Z

Not sure why the sftp fallback is being removed, and would like to hold merge until that is answered. Aside from that, looks ok.

This commit re-adds the logic to ask for case-id if the user hasn't specified it in the command line explicitly. Related: RHEL-92071 Signed-off-by: Jose Castillo <jcastillo@redhat.com>

jcastill requested a review from pmoravec May 21, 2025 09:24

pmoravec reviewed May 21, 2025

View reviewed changes

TurboTurtle requested changes May 21, 2025

View reviewed changes

jcastill force-pushed the jcastillo-case-id-upload-protocol branch from 928e759 to ff75a91 Compare May 27, 2025 11:54

pmoravec reviewed May 27, 2025

View reviewed changes

Comment thread sos/policies/distros/__init__.py Outdated

jcastill force-pushed the jcastillo-case-id-upload-protocol branch from ff75a91 to 6d02c2b Compare May 27, 2025 12:05

pmoravec approved these changes May 27, 2025

View reviewed changes

pmoravec added Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer Kind/Bug Something is not working, and this fixes the issue labels May 27, 2025

jcastill requested a review from arif-ali May 29, 2025 07:07

arif-ali approved these changes May 29, 2025

View reviewed changes

arif-ali added Reviewed/Ready for Merge Has been reviewed, ready for merge and removed Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer labels May 29, 2025

jcastill force-pushed the jcastillo-case-id-upload-protocol branch from 6d02c2b to c73ab3c Compare May 30, 2025 08:20

[policy] Re-add logic to request case-id if not present

b8ec339

This commit re-adds the logic to ask for case-id if the user hasn't specified it in the command line explicitly. Related: RHEL-92071 Signed-off-by: Jose Castillo <jcastillo@redhat.com>

jcastill force-pushed the jcastillo-case-id-upload-protocol branch from c73ab3c to b8ec339 Compare May 30, 2025 08:23

pmoravec approved these changes May 30, 2025

View reviewed changes

arif-ali approved these changes May 30, 2025

View reviewed changes

arif-ali merged commit 369e65a into sosreport:main May 30, 2025
39 checks passed

		_("Optionally, please enter the case id that you are "
		"generating this report for [%s]: ") % caseid

Uh oh!

Conversation

jcastill commented May 21, 2025

Uh oh!

packit-as-a-service Bot commented May 21, 2025

Uh oh!

pmoravec May 21, 2025

Choose a reason for hiding this comment

Uh oh!

pmoravec commented May 21, 2025

Uh oh!

TurboTurtle commented May 21, 2025

Uh oh!

TurboTurtle left a comment

Choose a reason for hiding this comment

Uh oh!

TurboTurtle May 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pmoravec commented May 28, 2025

Uh oh!

arif-ali commented May 29, 2025

Uh oh!

arif-ali left a comment

Choose a reason for hiding this comment

Uh oh!

arif-ali May 29, 2025

Choose a reason for hiding this comment

Uh oh!

TurboTurtle May 29, 2025

Choose a reason for hiding this comment

Uh oh!

pmoravec May 29, 2025

Choose a reason for hiding this comment

Uh oh!

jcastill May 30, 2025

Choose a reason for hiding this comment

Uh oh!

TurboTurtle commented May 29, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants