insights-core icon indicating copy to clipboard operation
insights-core copied to clipboard

feat: update instructions of the insights-client output

Open grunwmar opened this issue 1 year ago • 3 comments

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:

  • [x] Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • [x] Is this PR to correct an issue?
  • [x] 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.

grunwmar avatar Apr 30 '24 10:04 grunwmar

Can one of the admins verify this patch?

candlepin-bot avatar Apr 30 '24 10:04 candlepin-bot

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 avatar May 13 '24 10:05 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 avatar May 13 '24 16:05 DuckBoss

May I know if this PR ready for merge?

xiangce avatar Aug 02 '24 01:08 xiangce

Hi @xiangce thanks for checking in, this PR requires some additional changes. It's not ready to merge.

DuckBoss avatar Aug 02 '24 07:08 DuckBoss

Hi @mgrunwal - The PR was closed because that your fork was deleted, Please get the changes and re-open it in another new PR. Sorry for the inconvenience.

xiangce avatar Sep 14 '24 07:09 xiangce