-
Notifications
You must be signed in to change notification settings - Fork 114
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
[RHCloud] Update configure_rhai_client registration #16497
[RHCloud] Update configure_rhai_client registration #16497
Conversation
trigger: test-robottelo |
PRT Result
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know what is the error in registration specifically?
I just wonder if it was failed insights-client
installation during GR, which would explain why it works now when GR is done after client pre-installation.
Otherwise I agree to exercise the insights-client --register
via GR rather than separately.
@@ -1084,6 +1073,17 @@ def configure_rhai_client( | |||
# Ensure insights-client rpm is installed | |||
if self.execute('yum install -y insights-client').status != 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be moved to below the registration to check if insights-client package is installed?
and it can be installed with yum when register=False
>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, I think logically it should stay where it is... We need to make sure those packages are installed before we register
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ColeHiggins2 yes, but this step would be covered with setup_insights=register_insights
when register=True
, where this isn't required, but its required to install insights-client manually, when register=False
, right?
@@ -1052,17 +1052,6 @@ def configure_rhai_client( | |||
:param register: Whether to register client to insights |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use the same PR to describe use of register_insights
param here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its a setting when registering a client to a sat. You can set up insights during this process. This is true by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ColeHiggins2 yes, but I meant if it could be described in the function metadata, which it isn't
I honestly dont know what changed between global registration and the prior. I was hoping to hear back from @shweta83 or @tpapaioa on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix looks good to me. My only request is to update the method's name and docstring to remove references to rhai / red hat access insights, which is the old package name. configure_rhai_client
could instead be configure_insights_client
.
916edff
to
0f40e01
Compare
trigger: test-robottelo |
PRT Result
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to understand what changed here, but I don't think it's necessary, as this patch is logically sound.
@sambible There is no point merging the PRs if the threads are open and discussions are happening, especially if the code owners require Tier2 review. OR if you feel the feedback isn't necessary or non-blocking, at least you should comment on those threads. |
* Update configure_rhai_client registration * Update name and usage
Registering a host with insights client was failing.
After further investigation, It appears that we were are not actually syncing rhel 6 - 9 repos but instead "mocking" them to be enabled...which are needed for insights setup.
This process must have started failing during our update to global registration across components?
My solution to this is to simply move the registration portion of
confiure_rhai_client
to be executed AFTER we setup the correct "mock" repositories needed and packages installed.The other option in my mind was to manually register insights after we setup the repos using
insights-client --register
But the first way allows us to register insights upon Satellite registration (which is the expected behavior)