-
Notifications
You must be signed in to change notification settings - Fork 261
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
core: Quota name in audit log when deleting quota #481
Conversation
Added missing method for ${QuotaName} substitution in the audit log message for RemoveQuotaCommand. Change-Id: I9826e1965b7faf8dc46afa80d1218ae49fc4a34b Bug-Url: https://bugzilla.redhat.com/2018412 Signed-off-by: Shmuel Melamud <smelamud@redhat.com>
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 changes seems fine and simple.
But, should AuditLog take it from the quota itself? getQuota.getName()? without this additional getQuotaName()
?
@liranr23 That's how these substitutions work. There is no way to use expressions there. |
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.
yes, it is simple but wouldn't it make more sense to change RemoveQuotaCommand to extend QuotaCRUDCommand?
I don't see much sense in it. Besides |
Not for reusability but to represent kind-of relationship, after all CRUD includes "delete".. :) |
Yeah, but there is another problem: As an alternative, we can just rename |
practically, that missing variable is probably the only gap we'll have but conceptually it's not different from RemoveAffinityGroupCommand that extends AffinityGroupCRUDCommand - the parameters of the latter contain more data than the former needs (as you wrote, "remove" only needs the ID) so isn't it just about removing the NotNull for quota in QuotaCRUDParameters? |
@ahadas Won't removing OK, let me try with |
probably, I meant to do that and add a null-check where appropriate
ack |
Change-Id: I4af00ee5ce7c170d9ce07d9f63312166a1ed9fab Signed-off-by: Shmuel Melamud <smelamud@redhat.com>
Done. |
/ost |
Added missing method for ${QuotaName} substitution in the audit log
message for RemoveQuotaCommand.
Change-Id: I9826e1965b7faf8dc46afa80d1218ae49fc4a34b
Bug-Url: https://bugzilla.redhat.com/2018412
Signed-off-by: Shmuel Melamud smelamud@redhat.com