ovirt-engine
ovirt-engine copied to clipboard
oVirt
restapi: incorrect href for parent href
If User send REST API request for ovirt-engine/api/disks/{diskid}/disksnapshots?include_active&include_template he will get this incorrect href back
Bug-Url: https://bugzilla.redhat.com/2013697 Signed-off-by: Artiom Divak [email protected]
Additional info: Note that when 'include_template' flag is requested, and one of the disk_snapshots have a template disk as a parent (as described here https://bugzilla.redhat.com/1900597) - this template disk snapshot belongs to another disk, thus the href should be built accordingly.
Have you checked this scenario? In this case, the parent snapshot will have another diskId, thus the URL will be incorrect.
As I see, similar change is already done by https://github.com/oVirt/ovirt-engine/commit/b1579afed5a1ad0f9e844e1b5870280c71d37aa9
As I see, similar change is already done by b1579af
right, that was for a particular snapshot and this one is for a collection of snapshots
If User send REST API request for ovirt-engine/api/disks/{diskid}/disksnapshots?include_active&include_template he will get this incorrect href back . Now it will get correct output that will look like this .
Bug-Url: https://bugzilla.redhat.com/2013697 Signed-off-by: Artiom Divak [email protected]
Looks like the PR description is cut.
Please add the bad and good href examples, as appear in the commit message :)
much better now, thanks minor comments inside I'm still not convinced that we should set the parent link to the template's image when include-template is not set - until now I only thought if it's a proper design but now I wonder if it could lead to backward compatibility issues for clients that use that link (they'll see a link to an image that is not returned). @ArtiomDivak @mkemel @bennyz @barpavel what do you think?
I think that this link is intended to fetch a specific disksnapshot. Thus returning it when include_template is not set makes even more sense - we don't have the parent image here in the returned array of disksnapshots, but here's a correct link that you can fetch it with. Also don't think that there would be any backward compatibility issues, since this href was never correct anyway.
Also don't think that there would be any backward compatibility issues, since this href was never correct anyway.
@mkemel so previously when include-template was not set we set an incorrect href for the parent of the top-level volume? wasn't it empty?
@mkemel so previously when include-template was not set we set an incorrect href for the parent of the top-level volume? wasn't it empty?
No, it wasn't empty. The parent ID is always there, it's just that the URL is built incorrectly. BTW, include_template is a fairly new feature, and was added exactly as a solution to the fact that parent pointed to a disksnapshot not in the list.
No, it wasn't empty. The parent ID is always there, it's just that the URL is built incorrectly.
ack, in this case, it's really a bug fix more than an enhancement that users can complain about, ok.