feat: update instructions of the insights-client output

[grunwmar]

For systems not registered or failing to upload, the suggested instructions are not appropriate. Replace outdated suggestions if system is not registered or failing to upload.

CARD ID: CCT-265

All Pull Requests:

Check all that apply:

• Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
• Is this PR to correct an issue?
• Is this PR an enhancement?

Complete Description of Additions/Changes:

I updated instructions of the insights-client output.
I replaced outdated suggestions if system is not registered or failing to upload with new one provided in Jira card for issue CCT-265.

[candlepin-bot]

Can one of the admins verify this patch?

[m-horky]

The first state (RHSM unregistered & Insights unregistered) seems correct, I only left a comment addressing the output formatting.

However, after the message is printed, I get one more line:

Machine-id found, insights-client can not be registered. Please, unregister insights-client first: `insights-client --unregister`

Is that intentional?

The second state (RHSM registered & Insights unregistered) doesn't seem to address the issue properly. The output seems to be

This host has not been registered. Use --register to register this host.

which is coming from

insights-core/insights/client/phase/v1.py

Lines 271 to 272 in cdb63eb

	logger.info('This host has not been registered. '
	'Use --register to register this host.')

, no matter whether legacy upload is enabled or disabled.

[m-horky]

1. I'd remove the newline before please ensure..., let's keep it on one line.
2. The output contains dots before Register with..., is that intentional? It feels strange.

. Register with subscription-manager
# subscription-manager register

. Register with insights-client
# insights-client --register

[DuckBoss]

I was typing up my review but you beat me to it @m-horky !

I have the same thoughts as Matyas here, and I believe there are supposed to be numbers preceding the dots:

1. Register with subscription-manager
# subscription-manager register

2. Register with insights-client
# insights-client --register

[grunwmar]

Hi,
Number typos are corrected.
Displaying of new/old message about cert/basic auth method is now treated by else-if statement checking value of config.authmethod.

[DuckBoss]

Thanks, I'll review this soon 👍

[DuckBoss]

Hi, I ran into some minor issues when testing this PR and I wanted to confirm if you're getting the same results.

Let's break down the expected scenarios:

Scenario 1 - The system is NOT configured for basic auth AND not registered with subscription-manager

For this scenario, when sub-man is not registered and the insights config is set to use cert auth
it is almost working as expected (except the minor typo with the numbers and I'm unsure if Machine id found... message should be included here)

Likewise I tested the opposite, when the system is configured for basic auth and not registered with sub-man, I believe the output should remain the same as it was before but instead the new text changes are displayed.
(i.e. auth method set to BASIC in the cfg and subman unregistered, but the message still displays the new registration instructions from scenario 1)

Scenario 2: The system is NOT configured for basic auth AND is registered with subscription-manager

For this scenario, when sub-man is registered and the insights config is set to use cert auth it works as expected.

However I tested the opposite here as well, when the system is configured for basic auth and registered with sub-man, the output should remain the same as it was before but the new text changes from scenario 1 are displayed.
(i.e. auth method set to BASIC in the cfg and subman registered, but the message displays the new registration instructions from scenario 1)

tl;dr -
Apart from minor typo fixes, if these message should only be for cert-based auth then the message shouldn't change for basic-auth unless there's some additional context I'm missing.

[DuckBoss]

Update: I've checked with Christian and the expected output for the behaviour of a system configured with basic auth should remain as it was (just a confirmation for what I mentioned in my review above ^^).

[DuckBoss]

LGTM after recent changes. 😄

Let's also check if this line is needed or not before we try to get this merged:

Machine-id found, insights-client can not be registered. Please, unregister insights-client first: `insights-client --unregister`

It doesn't mention it explicitly in the done criteria of this card, but let's make sure.