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.

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

fix: prefix job_id with J: for _get_recipe_info #289

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

skycastlelily
Copy link
Contributor

fix the tmt clean guest issue: teemtee/tmt#2796

@skycastlelily
Copy link
Contributor Author

The problem is:delete_host,checks whether host_id.startswith("J:"),while tmt get the job_id from _get_recipe_info ,and recipe['job_id'] doesn't has the J: prefix.

@skycastlelily
Copy link
Contributor Author

Hi,pvoborni,would you please review this merge request,which is tiny and clear,and related to the merge request:#286:)

@skycastlelily
Copy link
Contributor Author

Sorry for the late reply,I'm just back from state holiday vacation.

The problem lies in the line of tmt:https://github.com/teemtee/tmt/blob/main/tmt/steps/provision/mrack.py#L696,create_server dose return a job_id with J:,but the def create function in tmt will return the info from _get_repcipe_info,ie,no J:, so  it will pass a job_id without J" when it calls mrack provider's delete_host,and the error happens.My fault,I thought create_server will return a job_id without J:,will be more careful next time when I submit a merge request.That being said,IMO we could and also should fix the problem from mrack side,as we never know whether other users will do the same thing:use the job_id from _get_recipe_info.So I update the merge request, would please review it,when you get some time to spare?Thanks:)

Copy link
Contributor

@pvoborni pvoborni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT.

In general, I don't like much the "magic" around the "J:", it would be better to have the beaker provider more refactored so the logic around the id is not on 3 different places and ideally tmt could avoid using internal method for getting current state.

@pvoborni pvoborni merged commit f1e7590 into neoave:main Apr 8, 2024
16 checks passed
@skycastlelily
Copy link
Contributor Author

skycastlelily commented Apr 28, 2024 via email

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.

2 participants