Merge lp://staging/~robru/bileto/publish-job into lp://staging/bileto

Mostly code style issues and nitpicks, but two or three more serious issues.

Replies inline,thanks Martin

Answered to your replies, thanks!

The flashplugin-nonfree component issue is still a blocker. The rsync race looks like an actual bug, but not that important (so not a blocker). Do with the code style/obfuscation comments what you want.

Ok I've pushed commits to resolve the issues I agree with, I'm choosing not to block on the two things that are historical cruft that I've inherited (flashplugin-nonfree/universe and '0 means no version'), I'm open to revisiting those later but for now those things work and are already production-proven, and I need to move forward with this rollout. Thanks for the review Martin!

Robert Bruce Park [2016-08-22 16:06 -0000]:
> Can you elaborate what you mean by "recursively call the method on its direct children"?

Well, I don't know what this is doing, but you said that __getattr__()
has some magic to do that kind of iteration.

> The reason that __getattr__ is defined to pass methods down to
> children is that this is called from dozens of places (and many of
> those cases it isn't overloaded at all, it's just a direct pass
> through), so I'm not going to reimplement that in every single place
> that needs to pass through. I'll probably rename __getattr__() to
> call_on_all_packages().

Doing "__getattr__ = call_on_all_packages" and replacing half of the
calls with the other makes things even *more*
confusing/implicit/surprising, so better revert that bit or fix it
consistently.

> There is an rsync server running continuously on the bileto
> instance, and snakefruit polls it every 5 minutes looking for a new
> file to read. Bileto doesn't "start" rsync.

So I suggest using a lock file (in both the rsync process and bileto)
to ensure to not run rsync while bileto is updating that file tree.
Another common pattern is to do something like this:

  cp -al cur_tree new_tree
  <change new_tree>
  mv cur_tree old_tree; mv new_tree cur_tree

You can't atomically rename an existing (non-empty) directory, but at
least this is fairly quick and thus will work in almost every case.
However, lock files are cheaper, simpler, and completely robust.

> As I said this was decided by infinity and I'm not sure what would be correct.

Please ask him again what this should mean and add a big fat comment
about it. The concept of "upload flashplugin-nonfree into universe"
simply does not make sense -- asking questions about it might be some
weird LP quirk, but it should at least be documented. You yourself
don't even know what it does any more :-)

On Aug 22, 2016 10:15 PM, "Martin Pitt" <email address hidden> wrote:
>
> Robert Bruce Park [2016-08-22 16:06 -0000]:
> > Can you elaborate what you mean by "recursively call the method on its
direct children"?
>
> Well, I don't know what this is doing, but you said that __getattr__()
> has some magic to do that kind of iteration.

So a ticket can have many packages. When you "build a ticket" you want to
build each package. The manager class exists to ensure the classes
representing the packages are correctly instantiated and then passes
messages back and forth. So when you call manager.build() you're really
calling .build() on every package instance. Manager.__gettattr__() contains
the logic for calling the method, in parallel, on each instance.

In some cases there's some "global" logic that operates once on the whole
ticket rather than per package, and that's where you see those manager
methods that call __getattr__, they're processing the return values that
came back from each package, etc. But there are other cases where you might
call "manager.foo()" and foo doesn't exist at all, it goes straight to
package.foo() on each package. The idea was that the manager is a sort of
transparent wrapper around the package instances, you can call package
methods on it and it passes the calls down to each instance. Eg, it's an
explicit design goal that you would often call methods on the manager
instance that do not exist, which is why __getattr__ is defined. It's just
unfortunate that so many methods require a bit of intervention before /
after the __getattr__ logic that you see so many methods calling
__gettattr__ with their own method name.

>
> > The reason that __getattr__ is defined to pass methods down to
> > children is that this is called from dozens of places (and many of
> > those cases it isn't overloaded at all, it's just a direct pass
> > through), so I'm not going to reimplement that in every single place
> > that needs to pass through. I'll probably rename __getattr__() to
> > call_on_all_packages().
>
> Doing "__getattr__ = call_on_all_packages" and replacing half of the
> calls with the other makes things even *more*
> confusing/implicit/surprising, so better revert that bit or fix it
> consistently.

What do you mean replacing half the calls? Nothing is calling
"self.__getattr__()" any longer so it's perfectly consistent. __getattr__
is kept for the cases when a nonexistent method is called on the manager
class. That has to stay because they chain together, eg, you can call
"manager.foo().bar()" and that calls foo and bar on all package instances.
Getting rid of __getattr__ would result in code like
"manager.call_on_all_packages('foo').call_on_all_packages('bar')", eg,
redundant slop.

>
> > There is an rsync server running continuously on the bileto
> > instance, and snakefruit polls it every 5 minutes looking for a new
> > file to read. Bileto doesn't "start" rsync.
>
> So I suggest using a lock file (in both the rsync process and bileto)
> to ensure to not run rsync while bileto is updating that file tree.
> Another common pattern is to do something like this:
>
> cp -al cur_tree new_tree
> <change new_tree>
> mv cur_t...