search

neoave / mrack

Multicloud use-case based multihost async provisioner for CIs and testing during development
Apache License 2.0
11 stars 14 forks source link

feat: Add support for translanting %pre and main body part of kickstart #294

Closed skycastlelily closed 2 months ago

skycastlelily commented 2 months ago

This merge request is needed for adding support kickstart in tmt mrack plugin:)

skycastlelily commented 2 months ago

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 https://github.com/neoave/mrack/pull/294#discussion_r1660924543:

  • 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 https://github.com/neoave/mrack/pull/294#pullrequestreview-2151168243, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKFR23F265BOTUGYL6JTUALZKFBKHAVCNFSM6AAAAABKE6URZKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNJRGE3DQMRUGM . You are receiving this because you authored the thread.Message ID: @.***>

skycastlelily commented 2 months ago

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 https://github.com/neoave/mrack/pull/294#discussion_r1660924543:

  • 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 https://github.com/neoave/mrack/pull/294#pullrequestreview-2151168243, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKFR23F265BOTUGYL6JTUALZKFBKHAVCNFSM6AAAAABKE6URZKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNJRGE3DQMRUGM . You are receiving this because you authored the thread.Message ID: @.***>

skycastlelily commented 2 months ago

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 https://github.com/neoave/mrack/pull/294#discussion_r1662576565:

  • 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 https://github.com/neoave/mrack/pull/294#pullrequestreview-2153909325, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKFR23FNPXNA64WCENH5MN3ZKKV4FAVCNFSM6AAAAABKE6URZKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCNJTHEYDSMZSGU . You are receiving this because you authored the thread.Message ID: @.***>

skycastlelily commented 2 months ago

yes,that is desired output, beaker is able to translate more than one %post section:)

Hi @pvoborni ,please feel free to tell me if you have any other concerns:)

dav-pascual commented 2 months ago

yes,that is desired output, beaker is able to translate more than one %post section:)

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

skycastlelily commented 2 months ago

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 https://github.com/neoave/mrack/pull/294#issuecomment-2222622397, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKFR23FWB5KAZ6WESE3C4LLZLZP4RAVCNFSM6AAAAABKE6URZKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRSGYZDEMZZG4 . You are receiving this because you were mentioned.Message ID: @.***>

dav-pascual commented 2 months ago

Hi @skycastlelily! In principle, changes LGTM But before approving, please fix the test failures first FileNotFoundError: [Errno 2] No such file or directory: 'key_two'. You will probably need to mock that part. You can run pytest in your local env installing the deps from test-requirements.txt

skycastlelily commented 2 months ago

You will probably need to mock that part.

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.

You can run pytest in your local env installing the deps from test-requirements.txt

Thanks for your info,gonna to check:)

skycastlelily commented 2 months ago

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 <![CDATA[one-letter]]> lines

dav-pascual commented 2 months ago

You will probably need to mock that part.

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.

I had in mind something like a '@/patch' mock style for the open file call, but this works too.

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 lines

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?

skycastlelily commented 2 months ago

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?

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

And why is it necessary to sort the ssh keys?

To make the test pass:)Set is unordered, key_two's content may be printed before key_one's.

dav-pascual commented 2 months ago

@skycastlelily Great, thanks for the changes. Let's merge!

skycastlelily commented 2 months ago

Thanks:)

On Tue, Jul 16, 2024 at 3:54 PM David Pascual @.***> wrote:

Merged #294 https://github.com/neoave/mrack/pull/294 into main.

— Reply to this email directly, view it on GitHub https://github.com/neoave/mrack/pull/294#event-13516661580, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKFR23CHAGLMS7YJQ2XQZQDZMTGSBAVCNFSM6AAAAABKE6URZKVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJTGUYTMNRWGE2TQMA . You are receiving this because you were mentioned.Message ID: @.***>