The type name should probably be dict rather than json, as json will never be used for that purpose anyway

The indentation of that dict looks broken :)

The worker should really read all of this from its configuration rather than having it passed around as a message.

• The access to the same objstorage can be different from one worker to another
• The archival policy can have changed since we sent the messages in the queue
• db access can also be different from one worker to another
• the dbconn string can contain passwords that we don't really want to have flying around

All in all, all the director should enqueue is the list of objects that might need copies.

This is okay as a first step but a tiny refactoring of the database should allow us to do that server-side rather than client-side.

As said before, the archiver worker should read the storages, dbconn and archival policy from its config.

Three arguments to be read from config.

I don't think this comment is necessary.

transfers

We don't want an ongoing copy to end up as present here: that would mean that we might try to copy from a source that is not there.

If the "virtual status" is not used elsewhere, you should probably just fold the timeout logic in this function.

spurious debug print

Please make that archiver/director (therefore storing the configuration in /etc/softwareheritage/archiver/director.{ini,yml})

We do not want to pass configuration to workers via Celery, ever. Considering that, I think the add_config instance attribute should be removed. The only actual use of that is overriding the file configuration when running the director on the command line, and we can just override specific configuration items in the director class's __init__ method.

This makes the "worker arguments" just a plain list of objects that need archiving. No need for a wrapping dict, or anything else.

Let's look at that after this huge diff lands.

This argument is not needed any longer: the configuration file can be loaded from the default Software Heritage paths, that is in /etc or the user home directory.

The default for the command-line director should be to read the config file.

When you're setting defaults for the command-line args, you make the command line _always_ override the contents of the configuration file with the default configuration.

What you should do is:

• drop the defaults
• make the arguments non-mandatory
• only override the contents of the configuration file if the command line argument is passed.

You should not need to do that any longer

There, you can just pass the command-line config.

Please make that archiver/worker.

I don't think add_config is needed.

This function can be removed now.

task.delay(batch)

task(batch)

I'm sorry, but I'd rather we removed all the command line arguments than adding this complexity.

This function has you duplicate the configuration keys at three places (command line argument names, function argument names, string constants), and realistically it will never be used as we will deploy a configuration file with puppet anyway...

Down the line, we should make sure that our "config module" can generate a command-line parser, rather than doing stuff ad-hoc everywhere.

Can we please just scrap the command line parsing and finally land this diff?

There's a typo here

Is there still a point in doing this override?