kernelci / kci-dev

Tool for interact with KernelCI instances
https://kernelci.github.io/kci-dev/
GNU Lesser General Public License v2.1
1 stars 2 forks source link

feat(all): Add global parameters #4

Closed nuclearcat closed 1 month ago

nuclearcat commented 1 month ago

1)Moving load_toml to main file, as it is common for all files. 2)Adding context to forward instance and settings to subcommands 3)Modifying at least "commit" subcommand to support latest Pipeline API 4)Add automatic repo info retrieval for "commit" with option to override by cli arguments.

aliceinwire commented 1 month ago

can you please divide this pr in small change as is difficult to review all toghether

nuclearcat commented 1 month ago

ok

aliceinwire commented 1 month ago

also I don't understand why you want to merge commit and patch with a general kci-dev.toml instead of keeping the current configuration file and trying to move away from standard click usage implementing a ctx context

nuclearcat commented 1 month ago

settings have identical functionality, it make sense to make it somewhere single place instead of duplicating function on each subcommand. As i recall context also standard click feature, if you want to have global options that are identical for all subcommands (and instance+settings are indeed same for all subcommands).

nuclearcat commented 1 month ago

My point is that if we will have for example 4 subcommands (jobretry, commit, patch, patchset), we will have to duplicate --settings and load_toml in each file. So i am applying DRY principle and found way to make it shared, i think only way to do that is to make this option as global, as is it is mostly identical. Also it opens way to add convenient option --instance, where we can test first some feature on local instance, then make PR and test on staging, and then if all ok - production. This is somehow similar concept how kernelci.toml works in kci(kernelci-core).

aliceinwire commented 1 month ago

My point is that if we will have for example 4 subcommands (jobretry, commit, patch, patchset), we will have to duplicate --settings and load_toml in each file. So i am applying DRY principle and found way to make it shared, i think only way to do that is to make this option as global, as is it is mostly identical. Also it opens way to add convenient option --instance, where we can test first some feature on local instance, then make PR and test on staging, and then if all ok - production. This is somehow similar concept how kernelci.toml works in kci(kernelci-core).

my idea about this was to move such duplicated code to a sort of common library script file, and just call the kernelci.toml reading function on each sub-command as needed. This would make each sub-command more flexible but still preventing to duplicate code.

nuclearcat commented 1 month ago

My point is that if we will have for example 4 subcommands (jobretry, commit, patch, patchset), we will have to duplicate --settings and load_toml in each file. So i am applying DRY principle and found way to make it shared, i think only way to do that is to make this option as global, as is it is mostly identical. Also it opens way to add convenient option --instance, where we can test first some feature on local instance, then make PR and test on staging, and then if all ok - production. This is somehow similar concept how kernelci.toml works in kci(kernelci-core).

my idea about this was to move such duplicated code to a sort of common library script file, and just call the kernelci.toml reading function on each sub-command as needed. This would make each sub-command more flexible but still preventing to duplicate code.

Agree, if it is common function that still have to be present on each subcommand (for example API call wrapper). Also i think load_toml i can move to such common library. But i think if each subcommand have common settings file, --settings logically should be global option?

aliceinwire commented 1 month ago

My point is that if we will have for example 4 subcommands (jobretry, commit, patch, patchset), we will have to duplicate --settings and load_toml in each file. So i am applying DRY principle and found way to make it shared, i think only way to do that is to make this option as global, as is it is mostly identical. Also it opens way to add convenient option --instance, where we can test first some feature on local instance, then make PR and test on staging, and then if all ok - production. This is somehow similar concept how kernelci.toml works in kci(kernelci-core).

my idea about this was to move such duplicated code to a sort of common library script file, and just call the kernelci.toml reading function on each sub-command as needed. This would make each sub-command more flexible but still preventing to duplicate code.

Agree, if it is common function that still have to be present on each subcommand (for example API call wrapper). Also i think load_toml i can move to such common library. But i think if each subcommand have common settings file, --settings logically should be global option?

right, so I think we can go with your patch.

aliceinwire commented 1 month ago

This PR looks good just be sure to have reformatted everything with python black (https://black.readthedocs.io/en/stable/index.html)

nuclearcat commented 1 month ago

Regarding load_toml, to place in some common library, where do you prefer to put it / name this library? kci-dev/common.py kci-dev/libs/common.py? Sure common and libs can be changed. Would like to know your preference.

aliceinwire commented 1 month ago

kci-dev/libs/common.py looks better

thanks for helping on this

aliceinwire commented 1 month ago

I added isort and python black check with GitHub action through poetry could you rebase over main? the import order change could have created some conflicts For reference: https://github.com/kernelci/kci-dev/pull/8

nuclearcat commented 1 month ago

sure, will do today

nuclearcat commented 1 month ago

I will do by small bits for now, i will open separate PR for --instance support and few more things

aliceinwire commented 1 month ago

I will do by small bits for now, i will open separate PR for --instance support and few more things

thanks you