feat: Add support for translanting %pre and main body part of kickstart #294
Conversation
|
This merge request is needed for adding support kickstart in tmt mrack plugin:) |
There was a problem hiding this comment.
I just quickly read the code, providing initial comment.
I'm thinking that it might be good to cover this functionality with more unit tests as now it only tests only part of the new scenario.
Mrack's documentation kinda sucks. But this feature is not very obvious and thus I'm thinking that some docs might help. Somewhere in https://github.com/neoave/mrack/tree/main/docs/guides
|
Ah,right, really sorry for making that kind of mistake, I should not only
pay attention to the part tmt use,I will keep that in mind,sorry again:)
…On Mon, Jul 1, 2024 at 8:10 PM Petr Vobornik ***@***.***> wrote:
***@***.**** commented on this pull request.
I just quickly read the code, providing initial comment.
I'm thinking that it might be good to cover this functionality with more
unit tests as now it only tests only part of the new scenario.
Mrack's documentation kinda sucks. But this feature is not very obvious
and thus I'm thinking that some docs might help. Somewhere in
https://github.com/neoave/mrack/tree/main/docs/guides
------------------------------
In src/mrack/transformers/beaker.py
<#294 (comment)>:
>
- res_ks_append += ks_append
+ if pubkeys:
+ res_ks_list += self._allow_ssh_keys(pubkeys)
Isn't this executing the self._allow_ssh_keys for the second time for the
"else" block of the previous if/else? I wonder how difficult would be to
unit test this case.
—
Reply to this email directly, view it on GitHub
<#294 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23F265BOTUGYL6JTUALZKFBKHAVCNFSM6AAAAABKE6URZKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNJRGE3DQMRUGM>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Updated:)
…On Tue, Jul 2, 2024 at 11:20 AM Lili Nie ***@***.***> wrote:
Ah,right, really sorry for making that kind of mistake, I should not only
pay attention to the part tmt use,I will keep that in mind,sorry again:)
On Mon, Jul 1, 2024 at 8:10 PM Petr Vobornik ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
>
> I just quickly read the code, providing initial comment.
>
> I'm thinking that it might be good to cover this functionality with more
> unit tests as now it only tests only part of the new scenario.
>
> Mrack's documentation kinda sucks. But this feature is not very obvious
> and thus I'm thinking that some docs might help. Somewhere in
> https://github.com/neoave/mrack/tree/main/docs/guides
> ------------------------------
>
> In src/mrack/transformers/beaker.py
> <#294 (comment)>:
>
> >
> - res_ks_append += ks_append
> + if pubkeys:
> + res_ks_list += self._allow_ssh_keys(pubkeys)
>
> Isn't this executing the self._allow_ssh_keys for the second time for
> the "else" block of the previous if/else? I wonder how difficult would be
> to unit test this case.
>
> —
> Reply to this email directly, view it on GitHub
> <#294 (review)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AKFR23F265BOTUGYL6JTUALZKFBKHAVCNFSM6AAAAABKE6URZKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNJRGE3DQMRUGM>
> .
> You are receiving this because you authored the thread.Message ID:
> ***@***.***>
>
|
|
yes,that is desired output, beaker is able to translate more than one %post
section:)
…On Tue, Jul 2, 2024 at 9:51 PM Petr Vobornik ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/mrack/transformers/beaker.py
<#294 (comment)>:
> + res_ks = ks_append.get("script")
+ res_ks_post = ks_append.get("post-install")
+ if res_ks_pre:
+ if res_ks_pre.startswith("%pre"):
+ res_ks_list += [res_ks_pre]
+ else:
+ res_ks_list += ["%pre"] + [res_ks_pre] + ["%end"]
+ if res_ks:
+ res_ks_list += [res_ks]
+ if res_ks_post:
+ if res_ks_post.startswith("%post"):
+ res_ks_list += [res_ks_post]
+ else:
+ res_ks_list += ["%post"] + [res_ks_post] + ["%end"]
+ else:
+ res_ks_list = ["%post"]
I think in this case, when ssh keys are defined, there will be 2 %post
section. I don't know if it is correct. But might not be and thus
highlighting it.
—
Reply to this email directly, view it on GitHub
<#294 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23FNPXNA64WCENH5MN3ZKKV4FAVCNFSM6AAAAABKE6URZKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNJTHEYDSMZSGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Hi @pvoborni ,please feel free to tell me if you have any other concerns:) |
@skycastlelily He is OOO right now, so I will handle the review :) |
|
Hi,thanks for your info and help in advance:)
…On Thu, Jul 11, 2024 at 6:54 PM David Pascual ***@***.***> wrote:
yes,that is desired output, beaker is able to translate more than one
%post section:)
Hi @pvoborni <https://github.com/pvoborni> ,please feel free to tell me
if you have any other concerns:)
@skycastlelily <https://github.com/skycastlelily> He is OOO right now, so
I will handle the review :)
—
Reply to this email directly, view it on GitHub
<#294 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23FWB5KAZ6WESE3C4LLZLZP4RAVCNFSM6AAAAABKE6URZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRSGYZDEMZZG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Hi @skycastlelily! In principle, changes LGTM |
you mean mock _allow_ssh_keys ?By that way we won't be able to check whether _allow_ssh_keys works well or not, I have updated the test case to resolve the issue you mentioned, please feel free to tell me if that way is not expected.
Thanks for your info,gonna to check:) |
|
Updated again,should be good now:)Please note,the original doesn't work well,as it omits a comma after "\n".join(res_ks_append) ,as a result the job submit will have lots of <ks_append></ks_append><ks_append> lines |
I had in mind something like a '@/patch' mock style for the open file call, but this works too.
I'm not sure I have understood this lastest change. As far as I know, adding a comma to the list doesn't make any difference. What was wrong before that now it is not? And why is it necessary to sort the ssh keys? |
The comma is there because this line will make the ks_append string to a long list whose elements containing only one letter. This mr is good without that comma because I have brackets the strings with [], or we will get a job like this one
To make the test pass:)Set is unordered, key_two's content may be printed before key_one's. |
|
@skycastlelily Great, thanks for the changes. Let's merge! |
|
Thanks:)
…On Tue, Jul 16, 2024 at 3:54 PM David Pascual ***@***.***> wrote:
Merged #294 <#294> into main.
—
Reply to this email directly, view it on GitHub
<#294 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKFR23CHAGLMS7YJQ2XQZQDZMTGSBAVCNFSM6AAAAABKE6URZKVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJTGUYTMNRWGE2TQMA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
No description provided.