Tickets 262, 273 and 266

[Vitezslav Humpa]

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

[Josef Skladanka]

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(a)redhat.com&gt;
>  To: autoqa-devel(a)lists.fedorahosted.org
>  Sent: Friday, March 18, 2011 6:27:34 PM
>  Subject: Tickets 262, 273 and 266
>  Hi people,
>   
Snapped

[James Laska]

On Mon, 2011-03-21 at 07:55 -0400, Josef Skladanka wrote:
>  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). 
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

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

[Kamil Paral]

>  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?

[James Laska]

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

[Josef Skladanka]

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(a)redhat.com&gt;
>  To: autoqa-devel(a)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(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/autoqa-devel

[James Laska]

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...

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

[Kamil Paral]

>  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...
>  
>  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.

[James Laska]

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...
>  > 
>  > 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

[Vitezslav Humpa]

Hi Joza

>  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.
>   
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

[Vitezslav Humpa]

>  > 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.
>  >
>  
>  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

[James Laska]

On Tue, 2011-03-22 at 10:54 -0400, Vitezslav Humpa wrote:
>  > > 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.
>  > >
>  > 
>  > 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

[Kamil Paral]

>  
>  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?

[James Laska]

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

[Kamil Paral]

>  >
>  > /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.

[James Laska]

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