-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Send events to MQ when hosts are renamed. #504
Send events to MQ when hosts are renamed. #504
Conversation
0d6c8bf
to
f9eb441
Compare
Clean up merge commit via rebase and force push. |
f9eb441
to
9471f33
Compare
if old_name is not None and old_name != instance.name: | ||
obj = { | ||
"old_host": old_name, | ||
"new_host": instance.name, | ||
"action": "host_updated", | ||
} | ||
else: | ||
obj = { | ||
"host": instance.name, | ||
"action": "host_updated", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- What do you think about using the same field name whether we have the old name or not? E.g. "host", and "old_host" in addition if it is not None. Might be less confusing to parse.
- How about using the event name "host_renamed" when the host has been renamed? Unless that may also happen without pre_save being called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The reason I opted out of using the same field name (ie,
host
) is that I found it ambiguous. Ishost
the new hostname, or the old hostname the recipient knows? If you are going to update an entry based on the event, you need to useold_host
if and only ifold_host
is set, as that's the hostname you know, but in every other version of thehost_updated
event, you would usehost
as the host you know. It's not a big deal, but I felt it being inconsistent. - We can certainly use a new event name. That would make using
host
above easier as we can define the API for thehost_rename
event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- That makes sense, thanks
- Actually, I meant action and not event name. But on second thought let's keep the event name "host" and action "host_updated", that will allow us to extend the functionality in the future if there are other changes to hosts that we want to send events for.
- Captures the old name with pre_save. - Makes no assumtion on signal consistency in Django... Fixes unioslo#476
9471f33
to
954920d
Compare
Fixes #476