Hi people,
I have done some work on tickets 262, 273 and 266 - concerning change of hook architecture to watcher/event and some refactoring of the AutoQA script.
Basically, the idea was to split the hook directory and put watchers into separate directory from hooks - which'd be called events. Together with this came the need to actually completely eliminate the term "hook" from AutoQA project, which I have done partially - on the main level. "Hooks" appear pretty much everywhere, including tests, so I am waiting with complete refactoring to events until I have some feedback.
So right now: hook directory ceased to exist, autoqa script mentions hooks no longer including the help messages etc., autoqa script has been organized into functions/procedures to make more sense, makefile has been rewritten accordingly to install the new directories, autoqa.config and cron files has also been edited to reflect the new "architecture".
To my knowledge and testing, these changes in architecture made no changes/regressions to the functionality, however I need you guys to confirm, if that is true. Additionally, if you have some time, please go over the change - as this was my first thing to do with AutoQA - to check whether I didn't create some obvious trouble.
I have created a new remote branch origin/vhumpa, where the code is stored. Please checkout the origin/vhumpa an try to "make clean install" the project and run some tests as you are used to, to see, if the behavior is the same. Please also delete the "/etc/autoqa.conf" before the first install as this file doesn't get deleted during the clean (the /usr/share/autoqa also didn't used to, however the new /usr/share/autoqa/watchers and /usr/share/autoqa/events now do).
I must admit that I am so far not familiar with the usual change confirming / patching process - please, anybody let me know if there is something particular to do that I omitted.
Thank you team! -- Vita Humpa Red Hat Brno
Hi,
Just a few notes to #266:
1) I'd support changing test_vars['hook'] to test_vars['event'] or something similar, since we'd like to get rid of the 'hook' term overall, IIRC.
2) Connects to (1) - if you decide to change that, then you'll also need to change the control.autoqa files (specifically the "if hook not in [xyz, abc]" lines).
3) I know it's not a programming issue, but changing README to reflect current state of watcher/event parsing would be nice.
Other than that, I think like your changes very much.
Joza
----- Original Message -----
From: "Vitezslav Humpa" vhumpa@redhat.com To: autoqa-devel@lists.fedorahosted.org Sent: Friday, March 18, 2011 6:27:34 PM Subject: Tickets 262, 273 and 266 Hi people,
Snapped
On Mon, 2011-03-21 at 07:55 -0400, Josef Skladanka wrote:
Hi,
Just a few notes to #266:
I'd support changing test_vars['hook'] to test_vars['event'] or something similar, since we'd like to get rid of the 'hook' term overall, IIRC.
Connects to (1) - if you decide to change that, then you'll also need to change the control.autoqa files (specifically the "if hook not in [xyz, abc]" lines).
Possibly future enhancement, but these dictionary hook/event checks in control.autoqa files seem too susceptible to failure. At least, I keep messing them up :) Should we define a common method to use instead, or is this a useless optimization?
def check_autoqa_event_support(event, approved_events=[], required_kwargs[]): if isinstance(String, approved_events): approved_events = [approved_events] if isinstance(String, restricted_events=): restricted_events= = [restricted_events=]
if event in approved_events and not in restricted_events return True else: return False
- I know it's not a programming issue, but changing README to reflect current state of watcher/event parsing would be nice.
Good suggestion.
Thanks, James
Possibly future enhancement, but these dictionary hook/event checks in control.autoqa files seem too susceptible to failure. At least, I keep messing them up :) Should we define a common method to use instead, or is this a useless optimization?
def check_autoqa_event_support(event, approved_events=[], required_kwargs[]): if isinstance(String, approved_events): approved_events = [approved_events] if isinstance(String, restricted_events=): restricted_events= = [restricted_events=]
if event in approved_events and not in restricted_events return True else: return False
I'm a little confused. Where should this method be defined and where it should be used and how?
On Mon, 2011-03-21 at 10:08 -0400, Kamil Paral wrote:
Possibly future enhancement, but these dictionary hook/event checks in control.autoqa files seem too susceptible to failure. At least, I keep messing them up :) Should we define a common method to use instead, or is this a useless optimization?
def check_autoqa_event_support(event, approved_events=[], required_kwargs[]): if isinstance(String, approved_events): approved_events = [approved_events] if isinstance(String, restricted_events=): restricted_events= = [restricted_events=]
if event in approved_events and not in restricted_events return True else: return False
I'm a little confused. Where should this method be defined and where it should be used and how?
I don't know ... I intentionally left out those details. I suspect somewhere where the 'autoqa' script can load the library? The logic in a few of the control.autoqa files is getting more complex (only run for these hooks, with these args etc...). It seemed worth a quick inspection to see if there was anything worthwhile here to avoid over-complicating our control.autoqa files. Just tossing this out there for feedback, not a requirement for Vita's patchset.
Thanks, James
Which control files do you have in mind?
I belive I see your point of view - there would only be few variables defined in the control.autoqa, and autoqa script would feed those to the check_autoqa_event_support() to determine whether to schedule or not.
I must say, that I like the current concept a bit better, since it leaves me with more versatility in the 'decide whether to run or not' process (although there are some catches).
Note: I'm deffinitely not trying to cast your idea away - I'm merely not seeing the possible benefit at the moment.
J. ----- Original Message -----
From: "James Laska" jlaska@redhat.com To: autoqa-devel@lists.fedorahosted.org Sent: Monday, March 21, 2011 3:14:49 PM Subject: Re: Tickets 262, 273 and 266 On Mon, 2011-03-21 at 10:08 -0400, Kamil Paral wrote:
Possibly future enhancement, but these dictionary hook/event checks in control.autoqa files seem too susceptible to failure. At least, I keep messing them up :) Should we define a common method to use instead, or is this a useless optimization?
def check_autoqa_event_support(event, approved_events=[], required_kwargs[]): if isinstance(String, approved_events): approved_events = [approved_events] if isinstance(String, restricted_events=): restricted_events= = [restricted_events=]
if event in approved_events and not in restricted_events return True else: return False
I'm a little confused. Where should this method be defined and where it should be used and how?
I don't know ... I intentionally left out those details. I suspect somewhere where the 'autoqa' script can load the library? The logic in a few of the control.autoqa files is getting more complex (only run for these hooks, with these args etc...). It seemed worth a quick inspection to see if there was anything worthwhile here to avoid over-complicating our control.autoqa files. Just tossing this out there for feedback, not a requirement for Vita's patchset.
Thanks, James
autoqa-devel mailing list autoqa-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/autoqa-devel
On Mon, 2011-03-21 at 10:28 -0400, Josef Skladanka wrote:
Which control files do you have in mind?
I belive I see your point of view - there would only be few variables defined in the control.autoqa, and autoqa script would feed those to the check_autoqa_event_support() to determine whether to schedule or not.
I must say, that I like the current concept a bit better, since it leaves me with more versatility in the 'decide whether to run or not' process (although there are some catches).
I do like it, it is powerful! I was running into some issues while trying to figure out why some of the anaconda tests are not currently being scheduled. Some of the issues were due to too much going on in some of my control.autoqa files, and a poor understanding on my part of the different kwargs each event provides.
http://git.fedorahosted.org/git/?p=autoqa.git;a=blob;f=tests/compose_tree/co...
The logic is by no means complex. Thinking more, I think my concern is just about the power (and lack of specificity) of the **kwargs. So each template has to do ... autoqa_kwargs.get('something', False) == 'something'. Perhaps this will just naturally improve over time as we have more package-specific tests. And given the recent watcher change, I'm a little behind on double checking how a test signs up for a particular watcher.
Note: I'm definitely not trying to cast your idea away - I'm merely not seeing the possible benefit at the moment.
No worries ... if it's a bad idea, I fully expect it to be tossed away!
Thanks, James
I do like it, it is powerful! I was running into some issues while trying to figure out why some of the anaconda tests are not currently being scheduled. Some of the issues were due to too much going on in some of my control.autoqa files, and a poor understanding on my part of the different kwargs each event provides.
http://git.fedorahosted.org/git/?p=autoqa.git;a=blob;f=tests/compose_tree/co...
The logic is by no means complex. Thinking more, I think my concern is just about the power (and lack of specificity) of the **kwargs. So each template has to do ... autoqa_kwargs.get('something', False) == 'something'. Perhaps this will just naturally improve over time as we have more package-specific tests. And given the recent watcher change, I'm a little behind on double checking how a test signs up for a particular watcher.
The logic of setting "execute" is a little confusing. I plan to change it from negative logic (saying when we don't want to run) to positive logic (saying when we want to run). That should simplify it. Of course further enhancements are welcome. I currently like that it's procedural (executable code) instead of declarative (xml file-like definitions). We may need to change that in the future, but currently it suits us fine IMO.
On Mon, 2011-03-21 at 11:11 -0400, Kamil Paral wrote:
I do like it, it is powerful! I was running into some issues while trying to figure out why some of the anaconda tests are not currently being scheduled. Some of the issues were due to too much going on in some of my control.autoqa files, and a poor understanding on my part of the different kwargs each event provides.
http://git.fedorahosted.org/git/?p=autoqa.git;a=blob;f=tests/compose_tree/co...
The logic is by no means complex. Thinking more, I think my concern is just about the power (and lack of specificity) of the **kwargs. So each template has to do ... autoqa_kwargs.get('something', False) == 'something'. Perhaps this will just naturally improve over time as we have more package-specific tests. And given the recent watcher change, I'm a little behind on double checking how a test signs up for a particular watcher.
The logic of setting "execute" is a little confusing. I plan to change it from negative logic (saying when we don't want to run) to positive logic (saying when we want to run). That should simplify it. Of course further enhancements are welcome. I currently like that it's procedural (executable code) instead of declarative (xml file-like definitions). We may need to change that in the future, but currently it suits us fine IMO.
Definitely! I do like and take advantage of the procedural nature. My sense is that at some point, having that much power, might work against us. Meaning, perhaps contributors might want a simpler way to indicate when the test should run ... something more API-like?
But I think I'm getting waaay ahead of myself here. Apologies :)
Thanks, James
Hi Joza
- I'd support changing test_vars['hook'] to test_vars['event'] or
something similar, since we'd like to get rid of the 'hook' term overall, IIRC.
- Connects to (1) - if you decide to change that, then you'll also
need to change the control.autoqa files (specifically the "if hook not in [xyz, abc]" lines).
- I know it's not a programming issue, but changing README to reflect
current state of watcher/event parsing would be nice.
We've just went through the all the changes with Kamil and decided to go with all the three things above just like you recommended - basically to go "deeper" and really remove the occurrence of "hook" everywhere :)
+ couple more things - do not modify the clean behavior of Makefile (clean just the src dir) - edit specfile accordingly (Something I didn't do before) - rename the watchers all to watcher.py and rename their dirs as well
Thanks for the review! -- Vita Humpa
- I'd support changing test_vars['hook'] to test_vars['event'] or
something similar, since we'd like to get rid of the 'hook' term overall, IIRC.
- Connects to (1) - if you decide to change that, then you'll also
need to change the control.autoqa files (specifically the "if hook not in [xyz, abc]" lines).
- I know it's not a programming issue, but changing README to
reflect current state of watcher/event parsing would be nice.
We've just went through the all the changes with Kamil and decided to go with all the three things above just like you recommended - basically to go "deeper" and really remove the occurrence of "hook" everywhere :)
- couple more things
- do not modify the clean behavior of Makefile (clean just the src
dir)
- edit specfile accordingly (Something I didn't do before)
- rename the watchers all to watcher.py and rename their dirs as well
I have just done what is mentioned above. Basically the "hook" term was finally eliminated from entire project. Considering the spread usage in code about everywhere, I hope I did not screw up some functionality.
James, If you can, please check, whether the changes to the SPEC file are fine, I have ignored it before.
Only place where I did not mess with the "hook" term is the git-post-receive watcher,as I was not confident with my understanding of various uses of "hook" in it, so if you guys don't mind, I'd keep refactoring of watchers/git-post-receive/* files (+related parts of Makefile and specfile) on it's creator (James?).
All has been pushed to origin/vhumpa again.
Thanks! -- Vita Humpa
On Tue, 2011-03-22 at 10:54 -0400, Vitezslav Humpa wrote:
- I'd support changing test_vars['hook'] to test_vars['event'] or
something similar, since we'd like to get rid of the 'hook' term overall, IIRC.
- Connects to (1) - if you decide to change that, then you'll also
need to change the control.autoqa files (specifically the "if hook not in [xyz, abc]" lines).
- I know it's not a programming issue, but changing README to
reflect current state of watcher/event parsing would be nice.
We've just went through the all the changes with Kamil and decided to go with all the three things above just like you recommended - basically to go "deeper" and really remove the occurrence of "hook" everywhere :)
- couple more things
- do not modify the clean behavior of Makefile (clean just the src
dir)
- edit specfile accordingly (Something I didn't do before)
- rename the watchers all to watcher.py and rename their dirs as well
I have just done what is mentioned above. Basically the "hook" term was finally eliminated from entire project. Considering the spread usage in code about everywhere, I hope I did not screw up some functionality.
James, If you can, please check, whether the changes to the SPEC file are fine, I have ignored it before.
Double check the Makefile as well. If you can run 'make rpms' successfully, that should be good.
Only place where I did not mess with the "hook" term is the git-post-receive watcher,as I was not confident with my understanding of various uses of "hook" in it, so if you guys don't mind, I'd keep refactoring of watchers/git-post-receive/* files (+related parts of Makefile and specfile) on it's creator (James?).
All has been pushed to origin/vhumpa again.
Maybe this was mentioned earlier, apologies if I missed it. Why the renaming of the events to drop the "post-" string? The 'post/pre' string seems to be important to the event still?
Thanks, James
Maybe this was mentioned earlier, apologies if I missed it. Why the renaming of the events to drop the "post-" string? The 'post/pre' string seems to be important to the event still?
I recommended some renaming to vhumpa, mainly for watcher names. But I believe we agreed to keep the event names the same, and they seem to still be the same:
/home/autoqa (vhumpa)$ ls -1 events/ git-post-receive post-bodhi-update post-bodhi-update-batch post-koji-build post-koji-build-batch post-repo-update post-tree-compose
On the other hand, the watchers are now named differently:
/home/autoqa (vhumpa)$ ls -1 watchers/ git-post-receive koji-bodhi repo-update tree-compose
I feel it just reads better.
Is that what you mean, or you had something else on mind?
On Tue, 2011-03-22 at 17:16 -0400, Kamil Paral wrote:
Maybe this was mentioned earlier, apologies if I missed it. Why the renaming of the events to drop the "post-" string? The 'post/pre' string seems to be important to the event still?
I recommended some renaming to vhumpa, mainly for watcher names. But I believe we agreed to keep the event names the same, and they seem to still be the same:
/home/autoqa (vhumpa)$ ls -1 events/ git-post-receive post-bodhi-update post-bodhi-update-batch post-koji-build post-koji-build-batch post-repo-update post-tree-compose
On the other hand, the watchers are now named differently:
/home/autoqa (vhumpa)$ ls -1 watchers/ git-post-receive koji-bodhi
Should this just be koji, we aren't really watching bodhi anymore, right?
repo-update tree-compose
I feel it just reads better.
Is that what you mean, or you had something else on mind?
I understand now, thanks. I just wasn't sure why the "post-" string was removed.
Thanks, James
/home/autoqa (vhumpa)$ ls -1 events/ git-post-receive post-bodhi-update post-bodhi-update-batch post-koji-build post-koji-build-batch post-repo-update post-tree-compose
On the other hand, the watchers are now named differently:
/home/autoqa (vhumpa)$ ls -1 watchers/ git-post-receive koji-bodhi
Should this just be koji, we aren't really watching bodhi anymore, right?
Implementation works with just koji tags, yes. But from the user perspective this watcher's purpose is to announce koji and bodhi events, so that's the reason I proposed this name.
repo-update tree-compose
I feel it just reads better.
Is that what you mean, or you had something else on mind?
I understand now, thanks. I just wasn't sure why the "post-" string was removed.
To emphasize that there is no 1:1 mapping before watcher and event.
Actually, maybe we should name them this way: watchers/git watchers/koji-bodhi watchers/yum-repo watchers/composes
git watcher will probably announce any git events (maybe there will be some other besides git-post-receive). koji-bodhi watcher announces koji and bodhi events. yum-repo watcher announces everything related to yum repositories (post-repo-update). and composes watcher handles composes events (post-tree-compose).
But they are *just* names, so if you think some different naming (or the original naming) is better, I've no hard feelings here. My intention was to make them easy to understand without much previous knowledge.
On Wed, 2011-03-23 at 08:49 -0400, Kamil Paral wrote:
/home/autoqa (vhumpa)$ ls -1 events/ git-post-receive post-bodhi-update post-bodhi-update-batch post-koji-build post-koji-build-batch post-repo-update post-tree-compose
On the other hand, the watchers are now named differently:
/home/autoqa (vhumpa)$ ls -1 watchers/ git-post-receive koji-bodhi
Should this just be koji, we aren't really watching bodhi anymore, right?
Implementation works with just koji tags, yes. But from the user perspective this watcher's purpose is to announce koji and bodhi events, so that's the reason I proposed this name.
That makes sense.
repo-update tree-compose
I feel it just reads better.
Is that what you mean, or you had something else on mind?
I understand now, thanks. I just wasn't sure why the "post-" string was removed.
To emphasize that there is no 1:1 mapping before watcher and event.
Actually, maybe we should name them this way: watchers/git watchers/koji-bodhi watchers/yum-repo watchers/composes
git watcher will probably announce any git events (maybe there will be some other besides git-post-receive). koji-bodhi watcher announces koji and bodhi events. yum-repo watcher announces everything related to yum repositories (post-repo-update). and composes watcher handles composes events (post-tree-compose).
I'm okay calling it whatever you feel is appropriate in the new scheme. Note, this particular watcher is designed to only trigger for the git 'post-receive' hook (http://www.kernel.org/pub/software/scm/git/docs/githooks.html#post-receive). The git server, when properly configured, will initiate this event (so it's a push, not a poll like other watchers). Many other git hooks are client-side ... and would likely involve a different watcher/approach for detection. I suspect they'd go into a different watcher, but too hard to tell at this time without a use case.
But they are *just* names, so if you think some different naming (or the original naming) is better, I've no hard feelings here. My intention was to make them easy to understand without much previous knowledge.
Can't argue with that goal :)
Thanks, James
autoqa-devel@lists.fedorahosted.org