Skip to content
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.

Sign up for GitHub

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

Jump to bottom

preserve the build sanity server for debugging #16276

Closed
wants to merge 1 commit into from
Closed

preserve the build sanity server for debugging #16276

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pytest_fixtures/core/sat_cap_factory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Contributor

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.

Copy link
Author

@omkarkhatavkar omkarkhatavkar Sep 9, 2024 •
edited
Loading

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,

Copy link
Member

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?

Copy link
Contributor

@jameerpathan111 jameerpathan111 Sep 10, 2024 •
edited
Loading

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.

Copy link
Contributor

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.

Copy link
Member

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()

sat = Satellite.get_host_by_hostname(sat.hostname)
sat.unregister()
Broker(hosts=[sat]).checkin()
Loading