Skip to content

fix(client): Update log messages when unregistered #4421

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pkoprda
Copy link
Contributor

@pkoprda pkoprda commented Apr 11, 2025

All Pull Requests:

Check all that apply:

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

Complete Description of Additions/Changes:

  • Card ID: CCT-265

This patch updates the logging messages shown to the user when the host is not registered with subscription-manager or when the host is not registered with insights-client.

@pkoprda pkoprda force-pushed the update-log-messages branch from 83d999b to ce05bc4 Compare April 11, 2025 15:21
@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 51.85185% with 13 lines in your changes missing coverage. Please review.

Project coverage is 77.63%. Comparing base (8f6ee99) to head (b7c8524).

Files with missing lines Patch % Lines
insights/client/phase/v1.py 64.70% 3 Missing and 3 partials ⚠️
insights/client/connection.py 37.50% 5 Missing ⚠️
insights/client/__init__.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4421      +/-   ##
==========================================
- Coverage   77.63%   77.63%   -0.01%     
==========================================
  Files         745      745              
  Lines       41623    41631       +8     
  Branches     6675     6678       +3     
==========================================
+ Hits        32313    32319       +6     
- Misses       8276     8278       +2     
  Partials     1034     1034              
Flag Coverage Δ
unittests 77.61% <51.85%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pkoprda pkoprda marked this pull request as ready for review April 11, 2025 15:24
@pkoprda pkoprda force-pushed the update-log-messages branch 3 times, most recently from 6695d5d to ceff6db Compare April 23, 2025 09:12
@m-horky
Copy link
Contributor

m-horky commented Apr 23, 2025

When not registered,

cat .../insights-client/integration-tests/playbook_verifier/playbooks/bugs.yml | insights-client -m insights.client.apps.ansible.playbook_verifier

displays the new message; instead, the Verifier should have been run.


We might have problems with the new wording of the message.

# legacy
System is NOT registered locally via .registered file.
# non-legacy
This host is unregistered.
# PR
This host has not yet been registered, [...]

Various integrations parse insights-client output, as the exit codes haven't been consistent before. rhc wouldn't affected, but the rhc system role would:

https://github.com/linux-system-roles/rhc/blob/41fd39f1741e8ce3ac3405662194ecd631d24e2b/tasks/insights-client.yml#L136-L144

Can you change the wording so it matches whatever is searched for in the role? We've since made the behavior consistent, and when not registered we always return non-zero code, but the role is ignoring that.

I think we should try to run the collection's tests before merging this.

@pkoprda pkoprda force-pushed the update-log-messages branch 3 times, most recently from c2c7484 to ff4ecc5 Compare April 24, 2025 11:59
* Card ID: CCT-265

This patch updates the logging messages shown to the user when the host
is not registered with subscription-manager or when the host is not
registered with insights-client.

Signed-off-by: pkoprda <[email protected]>
@pkoprda pkoprda force-pushed the update-log-messages branch from ff4ecc5 to b7c8524 Compare May 2, 2025 14:57
@zpetrace
Copy link

zpetrace commented May 5, 2025

Verification PASSED from the QE side, let's wait for a DEV ack and then we can merge:)

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

Successfully merging this pull request may close these issues.

4 participants