kparal and I had a discussion in IRC around the changes currently proposed in my patch for #265 - Create function to download RPMs from koji for given NVR. I am summarizing the conversation here.
- koji_utils is an extraordinarily difficult class to test due to the way that it is currently written and as such, was a poor choice for a testing proof-of-concept.
- The refactoring in koji_utils decreases the functionality in the class
- The advantages brought by the refactoring are not trivial to visualize and mostly affect the testability of the code
- tflink did miss a side-effect while testing that would break upgradepath
The current plan of action is: - tflink will rewrite the code for #265 in order to not change koji_utils
- testing will start with simpler code, if possible
- a ticket will be filed to deal with the testability issues in koji_utils.
Let us know if you have any other ideas/concerns/etc. I will be submitting a new patch for #265 in the next day or so.
Tim
On Mon, 2011-03-14 at 13:20 -0600, Tim Flink wrote:
kparal and I had a discussion in IRC around the changes currently proposed in my patch for #265 - Create function to download RPMs from koji for given NVR. I am summarizing the conversation here.
Hi guys, I just read this here and thought it'd be good to share that we have infrastructure in kvm autotest that does precisely this. We have improved it and it's now called KojiClient. It can download/install rpms downloaded from the build system. So as autoqa uses autotest, perhaps it could re-use this function?
Note that the final KojiClient form is under review, not commited upstream. As of now the class is KojiDownloader and it is located under client/tests/kvm/kvm_utils, and we are refactoring the kvm autotest to move those utility functions to an autotest shared library area.
Well, it's just an idea :) Let me know what you guys think.
koji_utils is an extraordinarily difficult class to test due to the way that it is currently written and as such, was a poor choice for a testing proof-of-concept.
The refactoring in koji_utils decreases the functionality in the class
The advantages brought by the refactoring are not trivial to visualize and mostly affect the testability of the code
tflink did miss a side-effect while testing that would break upgradepath
The current plan of action is:
tflink will rewrite the code for #265 in order to not change koji_utils
testing will start with simpler code, if possible
a ticket will be filed to deal with the testability issues in koji_utils.
Let us know if you have any other ideas/concerns/etc. I will be submitting a new patch for #265 in the next day or so.
Tim
autoqa-devel mailing list autoqa-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/autoqa-devel
Hi guys, I just read this here and thought it'd be good to share that we have infrastructure in kvm autotest that does precisely this. We have improved it and it's now called KojiClient. It can download/install rpms downloaded from the build system. So as autoqa uses autotest, perhaps it could re-use this function?
Note that the final KojiClient form is under review, not commited upstream. As of now the class is KojiDownloader and it is located under client/tests/kvm/kvm_utils, and we are refactoring the kvm autotest to move those utility functions to an autotest shared library area.
Well, it's just an idea :) Let me know what you guys think.
Hey Lucas, thanks for your comment! Here's direct link to the code:
http://autotest.kernel.org/browser/trunk/client/tests/kvm/kvm_utils.py#L1538
As for this particular piece of code, I think we will go with our own implementation, because it's pretty simple (few lines of code) and there a few details we'd need to change in your code, like the possibility to adjust Koji server URL (but I've just questioned that in ticket #280). But Tim can certainly look through it whether something interesting catches his eye.
What caught my attention more is a lot of other functions available in that file. It is certainly good to be aware of them while we try to implement some VM-related tests in the future. So thanks again!
Kamil
On 03/15/2011 03:59 AM, Kamil Paral wrote:
Hi guys, I just read this here and thought it'd be good to share that we have infrastructure in kvm autotest that does precisely this. We have improved it and it's now called KojiClient. It can download/install rpms downloaded from the build system. So as autoqa uses autotest, perhaps it could re-use this function?
Note that the final KojiClient form is under review, not commited upstream. As of now the class is KojiDownloader and it is located under client/tests/kvm/kvm_utils, and we are refactoring the kvm autotest to move those utility functions to an autotest shared library area.
Well, it's just an idea :) Let me know what you guys think.
Hey Lucas, thanks for your comment! Here's direct link to the code:
http://autotest.kernel.org/browser/trunk/client/tests/kvm/kvm_utils.py#L1538
As for this particular piece of code, I think we will go with our own implementation, because it's pretty simple (few lines of code) and there a few details we'd need to change in your code, like the possibility to adjust Koji server URL (but I've just questioned that in ticket #280). But Tim can certainly look through it whether something interesting catches his eye.
What caught my attention more is a lot of other functions available in that file. It is certainly good to be aware of them while we try to implement some VM-related tests in the future. So thanks again!
Kamil
Lucas,
Thanks for the heads up, we'll definitely keep that in mind as we move forward.
I agree with Kamil on the use of KojiDownloader, though. One of the major concerns in the recent code review was the immediate loss of functionality in moving from the full koji client implementation and KojiDownloader seems to be a move farther in that direction.
I like the simplicity and clean interface, though and we may want to move in more of that direction in the future.
Is the KojiClient code under review very different from the KojiDownloader code that is already in the autotest codebase?
Thanks again for the information, we appreciate it!
Tim
On Tue, 2011-03-15 at 10:46 -0600, Tim Flink wrote:
On 03/15/2011 03:59 AM, Kamil Paral wrote:
Hi guys, I just read this here and thought it'd be good to share that we have infrastructure in kvm autotest that does precisely this. We have improved it and it's now called KojiClient. It can download/install rpms downloaded from the build system. So as autoqa uses autotest, perhaps it could re-use this function?
Note that the final KojiClient form is under review, not commited upstream. As of now the class is KojiDownloader and it is located under client/tests/kvm/kvm_utils, and we are refactoring the kvm autotest to move those utility functions to an autotest shared library area.
Well, it's just an idea :) Let me know what you guys think.
Hey Lucas, thanks for your comment! Here's direct link to the code:
http://autotest.kernel.org/browser/trunk/client/tests/kvm/kvm_utils.py#L1538
As for this particular piece of code, I think we will go with our own implementation, because it's pretty simple (few lines of code) and there a few details we'd need to change in your code, like the possibility to adjust Koji server URL (but I've just questioned that in ticket #280). But Tim can certainly look through it whether something interesting catches his eye.
What caught my attention more is a lot of other functions available in that file. It is certainly good to be aware of them while we try to implement some VM-related tests in the future. So thanks again!
Kamil
Lucas,
Thanks for the heads up, we'll definitely keep that in mind as we move forward.
I agree with Kamil on the use of KojiDownloader, though. One of the major concerns in the recent code review was the immediate loss of functionality in moving from the full koji client implementation and KojiDownloader seems to be a move farther in that direction.
I like the simplicity and clean interface, though and we may want to move in more of that direction in the future.
Is the KojiClient code under review very different from the KojiDownloader code that is already in the autotest codebase?
Not much, it's an incremental improvement that we came up with after months of using that code:
1) It optionally parses a new configuration we have called PkgSpec, which is a better way to specify groups of packages to be downloaded 2) It optionally can install packages downloaded from the build system rather than just download them (why we don't call it KojiDownloader anymore) 3) Code simplification/cleanup
No problem, feel free to use whatever you like or not :) I'll take a look at your implementation and see if we can improve ours with the ideas.
Cheers,
Lucas
autoqa-devel@lists.fedorahosted.org