host package_list fix

[pondrejk]

Problem Statement

similar problem to #13797 -- the output of the cli package search won't qualify as csv but otherwise can be processed all right by the parse_csv function

This pr adds an exception, fixes around 6 tests

[pondrejk]

trigger: test-robottelo
pytest: tests/foreman/cli/test_host.py -k test_positive_install_package_via_rex[rhel8]

[ogajduse]

Please see https://bugzilla.redhat.com/show_bug.cgi?id=2263041 for more context since it is indirectly related to Peter's patch here.

Ideally, we should expect hammer to provide a valid CSV format if we specify it / request it.

In this case, the is_csv function is rejecting the CSV since the hammer output has only one column.

Nvra\nrabbit-1.5.6-1.noarch\n

I propose to revert #13797 first and then #13737, and a local workaround to remove the extra line in the output of job-invocation create.

[jyejare]

@ogajduse You can still use the output from CSV function and do a local workaround of removing extra new line. Not sure why would you like to revert the entire PR for ?

How often do we have tables with one column ?

[ogajduse]

@jyejare Can you please be more specific? What is the CSV funciton?

[jyejare]

@ogajduse parse_csv function, btw I am agree with having local handling of non csv returns as is and hence removing all those workarounds putting in parse_csv function including this one.

[jyejare]

@ogajduse We could also create a separate filter/parser for such STDOUTs that does not fit into is_csv but could be parsed using CSV lib parser. That way we will have a good segregation and also fits into the defination of CSV and non-CSV!

Something like:

# A new function that we need to create for CSV-like strings outputs
if is_csv(string) or is_csvlike(string):
    <code_here_for_csv_parsing>

Now is_csvlike will search for specific regex pattern etc that fits the definition of is_csvlike function.

Thoughts ?

[ogajduse]

Case with one column, one line

How often do we have tables with one column ?

This does not seem to be the correct question that should be on the table. I would put the question: Does hammer spit out tables with one column and one line?

Let me put another question here.
Does it happen that hammer does not provide a clean CSV when we request it using --output=csv?

D 240205 15:24:04 hosts:145] my.sat.com executing command: b'LANG=en_US.UTF-8  hammer -v -u admin -p changeme --output=csv host package list --host-id="3" --search="name=rabbit" '
[D 240205 15:24:06 hosts:147] my.sat.com command result:
    stdout:
    Nvra
    rabbit-1.5.6-1.noarch
    
    stderr:
    (0, b'')
    status: 0

In this case, csv.reader that we use for reading CSV from STDOUT works correctly.

[ins] In [142]: text6 = 'Nvra\nrabbit-1.5.6-1.noarch\n'

[ins] In [143]: reader = csv.reader(text6.splitlines())

[ins] In [144]: for l in reader:
           ...:     print(l)
           ...: 
['Nvra']
['rabbit-1.5.6-1.noarch']

However, csv.Sniffer fails on the same input.

[ins] In [149]: text6 = 'Nvra\nrabbit-1.5.6-1.noarch\n'

[ins] In [150]: sniffer = csv.Sniffer()

[ins] In [151]: sniffer.sniff(text6)
---------------------------------------------------------------------------
Error                                     Traceback (most recent call last)
Cell In[151], line 1
----> 1 sniffer.sniff(text6)

File /usr/lib64/python3.11/csv.py:187, in Sniffer.sniff(self, sample, delimiters)
    183     delimiter, skipinitialspace = self._guess_delimiter(sample,
    184                                                         delimiters)
    186 if not delimiter:
--> 187     raise Error("Could not determine delimiter")
    189 class dialect(Dialect):
    190     _name = "sniffed"

Error: Could not determine delimiter

Expectations of parse_csv function

I would like us to define the expectations about the robottelo.cli.hammer.parse_csv function.
This method is called only from the robottelo.ssh module if we specify output_format = 'csv'. Therefore we expect hammer to return valid CSV on STDOUT and we expect parse_csv to parse it. With the recently added is_csv functionality, it can happen that parse_csv is not able to parse STDOUT and returns it untouched. What happens in this case is that parse_csv did not parse the CSV and provided string as its output instead of list that it is supposed to return.

My suggestion

My suggestion would be to not use is_csv function since it breaks the logic of parsing CSV format and also due to csv.Sniffer failing to parse single column CSV.

Case with an extra line printed on STDOUT when CSV/JSON is requested

In this case, the strict zip function breaks on zipping the last line that has three columns, whereas the table header has only two columns.

2023-05-27 12:42:42 - gw0 - broker - DEBUG - my.satellite.com command result:
    stdout:
    Message,Id
    Job invocation 2 created,2
    1 task(s), 1 success, 0 fail
    
    stderr:
    (574, b'Task d3bcdc09-2fd0-40ca-b390-b98ce258057b running: 0.17/1, 17%, elapsed: 00:00:00\nTask d3bcdc09-2fd0-40ca-b390-b98ce258057b running: 0.33/1, 33%, elapsed: 00:00:02\nTask d3bcdc09-2fd0-40ca-b390-b98ce258057b running: 0.33/1, 33%, elapsed: 00:00:04\nTask d3bcdc09-2fd0-40ca-b390-b98ce258057b running: 0.33/1, 33%, elapsed: 00:00:06\nTask d3bcdc09-2fd0-40ca-b390-b98ce258057b running: 0.33/1, 33%, elapsed: 00:00:08\nTask d3bcdc09-2fd0-40ca-b390-b98ce258057b success: 1.0/1, 100%, elapsed: 00:00:10\nTask d3bcdc09-2fd0-40ca-b390-b98ce258057b success: 1.0/1, 100%, elapsed: 00:00:10\n')
    status: 0

My suggestion

I would suggest to patch the command method to strip the last line so parse_csv can parse the STDOUT. This patch should be present until https://bugzilla.redhat.com/show_bug.cgi?id=2263041 gets resolved.

diff --git a/robottelo/ssh.py b/robottelo/ssh.py
index c82a7ebc0..d436b3b9b 100644
--- a/robottelo/ssh.py
+++ b/robottelo/ssh.py
@@ -52,6 +52,10 @@ def command(
     result = client.execute(cmd, timeout=timeout)
 
     if output_format and result.status == 0:
+        if result.stdout and "job-invocation create" in cmd:
+            # strip the last line of the output which is not csv:
+            # 1 task(s), 0 success, 1 fail
+            result.stdout = result.stdout[0:result.stdout.rfind("\n", 0, len(result.stdout) - 1) + 1]
         if output_format == 'csv':
             result.stdout = hammer.parse_csv(result.stdout) if result.stdout else {}
         if output_format == 'json':

Final words

I'm open to further discussion on finding the best solution for this issue. Looking forward to finding a solution that addresses our concerns while maintaining code consistency."

[ogajduse]

Let me clarify what I mean by revert. By revert I mean using git revert to create an inverse patch and commit it to the git history. I certainly did not mean to alter the git history by doing some obscure reverts by removing commits from the history.

In the current state, robottelo is masking a product bug https://bugzilla.redhat.com/show_bug.cgi?id=2263041. Introducing the flake8-bugbear rule https://docs.astral.sh/ruff/rules/zip-without-explicit-strict/ that made us use strict mode for zipping CSV header and lines together, revealed this product bug. Let's dive into the code.

Let's start from the method that wraps the hammer job-invocation create command.

robottelo/robottelo/cli/job_invocation.py

Lines 34 to 38 in 362c6e8

	@classmethod
	def create(cls, options):
	"""Create a job"""
	cls.command_sub = 'create'
	return cls.execute(cls._construct_command(options), output_format='csv')

We explicitly request hammer to provide output in CSV by output_format='csv'. The customer expects to receive ONLY the data in the requested format on stdout. If we expect hammer to provide the output in the base format, the human-readable only, we do not specify output_format='csv' and leave it set to None which results in our hammer wrapper not providing the --output argument to the hammer command. Therefore it is then up to us to work with that format and parse it on our own, but if we explicitly specify the format we want hammer to spit out the data in, we should rely on hammer in this. If hammer corrupts that data in any way, we should file a bug.
With that stated, parse_csv function should throw an exception in case it can not parse the CSV output - indicating a product bug.

I would like to focus on providing a real fix that would ensure that we are catching real product bugs instead of creating mechanisms that might potentially hide them.

[JacobCallahan]

I largely like what @ogajduse has proposed here. More accuracy is definitely better and the lack of identification of single column csv is a real problem with the current detection method.
However, I would like to see some mechanism to force on/off csv conversion on a per-command basis that would override any automatic detection methods replacing the current method.

Additionally, one nitpick on the proposed code for stripping the bad line. I think this version is more readable, but does not include a trailing newline character. I don't believe this would break the reader, but could be arguably less aesthetic than a preserved trailing newline.

result.stdout = "\n".join(result.stdout.splitlines()[:-1])

which results in

'Message,Id\nJob invocation 2 created,2'

compared to the proposed version's

'Message,Id\nJob invocation 2 created,2\n'

[jyejare]

@JacobCallahan @ogajduse

Lets see some background of why and what kind of STDOUTs we had to patch the #13737 for. In simple terms many STDOUTs arent fitting into the format of CSV and I agree that we as QE should not accept such faulty outputs and even customers who is willing to get CSV output demands from us to be the CSV only. Thanks @ogajduse for filing that bug though its nitpcik we could explain DEVs how this could affect customers if they are using CSV output for some automation and converting the STDOUT into CSV returning only half information (example is described in #13737).

So what does #13737 do is what @ogajduse suppose it to do. That means Tests are asking for CSV but the STDOUT is not eligible for CSV (verified by CSV modules own Sniffer module for it, but it seems that is not helping us with single column. My proposal is to right our own function for such pattrens to identify its eligible for CSV conversion), hence its returning the STDOUT as-is without conversions using CSV module(as its returning half fields in STDOUT formats) making the QE to decide whatever they wanted to do with non-CSV-convertible STDOUT. So they can either raise a bug or have test level workarounds that wont block there functional test case for such nitpick issue.

So, my opinion is to combining the approach of the Sniffer/Sniffing and develop our own isCSVlike function with REGEX patten identifying single column like stdout or more such formats those are convertible from CSV module and rest should be either not to be asked for CSV conversion or file a bug.

[pondrejk]

I'm putting this to draft. I'll be working on a solution to reflect all said above, will notify you in a separate PR

[pondrejk]

trigger: test-robottelo
pytest: tests/foreman/cli/test_host.py -k test_positive_install_package_via_rex[rhel8]

[pondrejk]

trigger: test-robottelo
pytest: tests/foreman/cli/test_host.py -k test_positive_install_package_via_rex[rhel8]

[pondrejk]

trigger: test-robottelo
pytest: tests/foreman/cli/test_host.py -k test_positive_install_package_via_rex[rhel8]