-
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
preserve the build sanity server for debugging #16276
preserve the build sanity server for debugging #16276
Conversation
@@ -387,7 +387,7 @@ def installer_satellite(request): | |||
configure_nailgun() | |||
configure_airgun() | |||
yield sat | |||
if 'sanity' not in request.config.option.markexpr: | |||
if 'sanity' not in request.config.option.markexpr and settings.server.auto_checkin: |
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.
@omkarkhatavkar If I'm reading this correctly, the checkin won't happen for the sanity server anyway.
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.
Maybe I confused you here, I'm talking here or this PR not want to check out the build_sanity satellite,
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.
If we run pytest with -m build_sanity
, then 'sanity' not in request.config.option.markexpr
evaluates to False
, therefore if settings.server.auto_checkin
is set to False
for the build sanity run, then we get
if False and False
which evaluates to False
. Therefore the following logical block will not get executed.
In a scenario where we run standard test automation, it would be if True and True
, therefore the sat
instance would be checked in unless we change the auto_checkin
setting to False.
I see no issue with the logic Omkar proposes. Are there any problems that might arise if this patch goes in, @jameerpathan111?
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.
I'm not saying that there'll be any problem but the PR description IMO is/was misleading as the built sanity server was not going to checked in anyway so I didn't see the point in having this code to solve the problem which is not there.
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.
Anyway, like you said I don't see any problems with having this patch either.
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.
I did not state it explicitly in the above comment, but @omkarkhatavkar, could you please revisit the logic? For sanity testing with auto_checkin
set to True, the condition would be if False and True
-> False
. Jameer is correct in what he is saying, there is no problem to solve, installer_satellite
is not being checked in in case of sanity testing.
If we want to do a VM check-in in robottelo, I would prefer to relo solely on the auto_checkin
setting. So the condition would look like
if settings.server.auto_checkin:
do_checkin()
@@ -387,7 +387,7 @@ def installer_satellite(request): | |||
configure_nailgun() | |||
configure_airgun() | |||
yield sat | |||
if 'sanity' not in request.config.option.markexpr: | |||
if 'sanity' not in request.config.option.markexpr and settings.server.auto_checkin: |
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.
If we run pytest with -m build_sanity
, then 'sanity' not in request.config.option.markexpr
evaluates to False
, therefore if settings.server.auto_checkin
is set to False
for the build sanity run, then we get
if False and False
which evaluates to False
. Therefore the following logical block will not get executed.
In a scenario where we run standard test automation, it would be if True and True
, therefore the sat
instance would be checked in unless we change the auto_checkin
setting to False.
I see no issue with the logic Omkar proposes. Are there any problems that might arise if this patch goes in, @jameerpathan111?
trigger: test-robottelo |
PRT Result
|
This change is invalid, closing this PR |
Problem Statement
Currently, the build sanity server is checked in as part of the pytest session, regardless of whether the build sanity test passes or fails. This process makes it challenging to identify the root cause of any issues immediately. To improve this, we propose delaying the check-in of the build sanity server and preserving it for a specified period. This will allow us to use the preserved instance to investigate the root cause of issues or provide it to the delivery team for further analysis.
Solution
This PR handles the preservation of the sanity server
Related Issues