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

Published 20 hours ago • verified publisher icon RedHatInsights

fix: insights-client failed, when --group was used

Open jirihnidek opened this issue 10 months ago • 7 comments

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?
  • [ ] Is this PR an enhancement?

Complete Description of Additions/Changes:

  • Card ID: CCT-186
  • When system was registered against Satellite server, and "insights-client --register" was triggered --group="tag" CLI option, then registration failed
  • Satellite also contains some bug, and it always returns status code 200 and empty list regardless existence of given group.

jirihnidek avatar Apr 03 '24 16:04 jirihnidek

FYI, please don't worry about the failure of CI/CD, it was not caused by this PR and will be fixed once the new uploader.json is released. Please ignore it for now.

xiangce avatar Apr 04 '24 10:04 xiangce

I can see that Jenkins jobs failed, but I cannot see, why did it fail.

jirihnidek avatar Apr 05 '24 08:04 jirihnidek

I can see that Jenkins jobs failed, but I cannot see, why did it fail.

Please ignore it too. It's a known issue mentioned in CCT-435. The QE team will fix it soon. Please ignore the other failures as well and start the review processes.

xiangce avatar Apr 07 '24 01:04 xiangce

Can one of the admins verify this patch?

candlepin-bot avatar Apr 26 '24 15:04 candlepin-bot

Would it be possible to create tests for this? Mock out some example responses the server can return. To be honest, I'm kind of lost in this method now, I'd like to see the difference between hosted and Satellite.

I will try to do that.

jirihnidek avatar Jun 10 '24 14:06 jirihnidek

Thank you. Would it be possible to add a test for the Satellite case as well? I'd like to have it covered as well. Just one more test that would show that response code 200 may not be enough.

What exactly do you mean to add test for the Satellite case?

jirihnidek avatar Jul 01 '24 07:07 jirihnidek

Satellite also contains some bug, and it always returns status code 200 and empty list regardless existence of given group.

Something that would mock a response like that -- server returning 200 with empty list. The method in connection.py grew a lot, but the tests don't don't seem to be covering the edge case your patch was addressing.

m-horky avatar Jul 01 '24 08:07 m-horky

May I know if this Pr ready for merge?

xiangce avatar Aug 02 '24 01:08 xiangce

@xiangce Hi, I believe we're waiting for QE to verify this patch. I'll ping you once it is ready.

m-horky avatar Aug 02 '24 07:08 m-horky

@xiangce Ready to be merged :+1:

m-horky avatar Aug 07 '24 15:08 m-horky