lookaside: Dehardcode some assumptions #170
Conversation
| try: | ||
| path = msg['msg']['path'] | ||
|
|
||
| except KeyError: |
There was a problem hiding this comment.
I really don't like the idea of try-than-fail.
How about just checking "'path' in msg['msg']"?
|
I'm not sure I prefer sending messages with just the path embedded completely: how about if people want to parse the filename or hashsum etc? I'd rather avoid having to tell them "just parse the path" if we have the information available when sending the message. I think a better way might be to replace the md5hash thing by something like This way, we could switch the hash algorithm without any trouble, and we would still get the information everywhere. |
|
I wasn't thinking of removing the informations currently in the message. People and tools trying to obtain informations like the checksum or the file name won't ever have to parse the path, they will continue to get them from the messages. What I'm doing here is remove broken assumptions in a particular consumer (this project), not remove data from the messages emitted. What you suggest (which is what I was already going to do on the emitter side actually) only helps for one assumption: the hash type. However, there is a second assumption, which is just as broken: that the path to the file is made of This second assumption is only true now, but it will be false when we finish the migration to another hash than MD5. (the hash type will be part of the directory structure, like so: Realistically, only the lookaside server truely knows its path structure, consumers should not assume they can recreate from what they think are its components. |
|
@pypingou just made an interesting observation: the fallback code can never ever be eliminated as we need to be able to parse old messages. So, as much as I prefer the « try new then fallback on the old in except » code (« it's easier to ask for forgiveness than permission »), it means that if we ever change again the messages in the future, we're going to have to deal with multiple exception levels. In such a case, asking for permission does make much more sense, so here's an updated pull request which does just that. 😃 |
|
Thanks for fixing that, and for re-assuring me that the checksum and filename etc attributes aren't removed. |
The current code assumes that: 1. the message contains a 'md5sum' key, which is going to go away when we move to a different hash algorithm, 2. it knows the path to the file on the lookaside cache, which is going to change very soon. The solution this change implements is to simply take the path to the uploaded source file entirely from the message. However, the lookaside cache doesn't emit messages like this yet, so the current code is kept as a fallback. https://fedorahosted.org/rel-eng/ticket/5846
|
Heh, turns out I broke the tests with a stupid syntax error. 😄 Fixed now (I just added a missing |
Yeah, this is already the case with some of the other processors. ;) |
|
👍 from me to merge. Thanks @bochecha, @puiterwijk, and @pypingou. |
lookaside: Dehardcode some assumptions
This is the counterpart of this change: fedora-infra/fedmsg_meta_fedora_infrastructure#170 Now that is has been deployed, we can start emitting the new messages.
The current code assumes that:
we move to a different hash algorithm,
to change very soon.
The solution this change implements is to simply take the path to the
uploaded source file entirely from the message.
However, the lookaside cache doesn't emit messages like this yet, so the
current code is kept as a fallback.
https://fedorahosted.org/rel-eng/ticket/5846