Re: [ovirt-devel] Making event types backwards compatible?

On May 30, 2017 11:28, "Juan Hernández" <jhernand(a)redhat.com <mailto:jhernand@redhat.com>> wrote: On 05/30/2017 09:38 AM, Tomas Jelinek wrote: > > > On Tue, May 30, 2017 at 9:16 AM, Juan Hernández <jhernand(a)redhat.com <mailto:jhernand@redhat.com> > <mailto:jhernand@redhat.com <mailto:jhernand@redhat.com>>> wrote: > > On 05/30/2017 08:55 AM, Tomas Jelinek wrote: > > > > > > On Tue, May 30, 2017 at 7:20 AM, Michal Skrivanek <mskrivan(a)redhat.com <mailto:mskrivan@redhat.com> <mailto:mskrivan@redhat.com <mailto:mskrivan@redhat.com>> > > <mailto:mskrivan@redhat.com <mailto:mskrivan@redhat.com> <mailto:mskrivan@redhat.com <mailto:mskrivan@redhat.com>>>> wrote: > > > > > On 29 May 2017, at 11:44, Juan Hernández <jhernand(a)redhat.com <mailto:jhernand@redhat.com> <mailto:jhernand@redhat.com <mailto:jhernand@redhat.com>> > <mailto:jhernand@redhat.com <mailto:jhernand@redhat.com> <mailto:jhernand@redhat.com <mailto:jhernand@redhat.com>>>> wrote: > > > > > >> On 05/29/2017 11:27 AM, Michal Skrivanek wrote: > > >> > > >>> On 29 May 2017, at 10:39, Juan Hernández <jhernand(a)redhat.com <mailto:jhernand@redhat.com> <mailto:jhernand@redhat.com <mailto:jhernand@redhat.com>> > > <mailto:jhernand@redhat.com <mailto:jhernand@redhat.com> <mailto:jhernand@redhat.com <mailto:jhernand@redhat.com>>>> wrote: > > >>> > > >>> Hello, > > >>> > > >>> It has been recently requested that the API provides event > types: > > >>> > > >>> [RFE] Expose event types to API > > >>> https://bugzilla.redhat.com/1453170 <https://bugzilla.redhat.com/1453170> > <https://bugzilla.redhat.com/1453170 <https://bugzilla.redhat.com/1453170>> > > <https://bugzilla.redhat.com/1453170 <https://bugzilla.redhat.com/1453170> > <https://bugzilla.redhat.com/1453170 <https://bugzilla.redhat.com/1453170>>> > > >>> > > >>> Currently the API provides the event code and description, for > > example: > > >>> > > >>> <event href="/ovirt-engine/api/events/8021" id="8021"> > > >>> <code>19</code> > > >>> <description>Host myhost failed to recover.</description > > >>> ... > > >>> </event> > > >>> > > >>> There is no documentation of what is the meaning of codes, > > except the > > >>> source code of the engine itself. This forces some > applications > > to add > > >>> their own code to name mapping. For example, the 'ovirt' Ruby > > gem used > > >>> by older versions of ManageIQ to interact with oVirt contains > > the following: > > >>> > > >>> > > > https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/event.rb#L25-L485 <https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/event.rb#L25-L48... > <https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/event.rb#L25-L485 <https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/event.rb#L25-L48... > > > <https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/event.rb#L25-L485 <https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/event.rb#L25-L48... > <https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/event.rb#L25-L485 <https://github.com/ManageIQ/ovirt/blob/v0.17.0/lib/ovirt/event.rb#L25-L48... > > >>> > > >>> We could avoid this by adding to the API a new event > attribute that > > >>> indicates the type: > > >>> > > >>> <event href="/ovirt-engine/api/events/8021" id="8021"> > > >>> <code>19</code> > > >>> <type>host_recover_failure</type> > > >>> <description>Host myhost failed to recover.</description> > > >>> ... > > >>> </event> > > >>> > > >>> Ideally this should be defined as an enum, so that it will be > > >>> represented as an enum in the SDKs. Alternatively it could > just > > be an > > >>> string, and we could reuse the 'name' attribute: > > >>> > > >>> <event href="/ovirt-engine/api/events/8021" id="8021"> > > >>> <code>19</code> > > >>> <name>host_recover_failure</name> > > >>> <description>Host myhost failed to recover.</description> > > >>> ... > > >>> </event> > > >>> > > >>> However, the key point to making this useful would be to keep > > the types > > >>> (or names) backwards compatible, so that users of the API can > > rely on > > >>> their values and meanings. > > >>> > > >>> So this is my question to you: can we commit to keep the > names and > > >>> meanings of the backend event types backwards compatible? > > >> > > >> Do we even have to make it bw compatible? > > >> I guess it depends on the actual usage of those names… > > >> The ovirt ruby gem itself doesn’t do much with it > > > > > > We need to make keep it backwards compatible or else tell > users "don't > > > rely on these values, as they may change without notice". > > > > > > The 'ovirt' gem doesn't do anything special, it just creates > its own > > > code to name mapping. But the users of the 'ovirt' gem (the > ManageIQ > > > oVirt provider) do rely on the name. For example: > > > > > > > > > > https://github.com/ManageIQ/manageiq-providers-ovirt/blob/master/app/mode... <https://github.com/ManageIQ/manageiq-providers-ovirt/blob/master/app/mode... > <https://github.com/ManageIQ/manageiq-providers-ovirt/blob/master/app/mode... <https://github.com/ManageIQ/manageiq-providers-ovirt/blob/master/app/mode... > > > <https://github.com/ManageIQ/manageiq-providers-ovirt/blob/master/app/mode... <https://github.com/ManageIQ/manageiq-providers-ovirt/blob/master/app/mode... > <https://github.com/ManageIQ/manageiq-providers-ovirt/blob/master/app/mode... <https://github.com/ManageIQ/manageiq-providers-ovirt/blob/master/app/mode... > > > > > > hmmm, while we are on topic, this pretty much looks like that manageiq > > does not only rely on the code but also on the actual value of it > since > > it is parsing it: > > > > # sample message: "Interface nic1 (VirtIO) was added to VM v5. (User: > > admin@internal-authz)" message.split(/\s/)[7][0...-1] > > > > Is this something we commit to maintain? Or should we commit to > maintain it? > > > > That is a good point, that isn't very future proof. We should also find > a way to make less fragile. Any suggestion? > > > The only doable thing which comes to my mind is something like this: > The msg is defined like this: > USER_ADD_VM_POOL_WITH_VMS_FAILED=Failed to create VM Pool ${VmPoolName} > (User: ${UserName}). > > e.g. the msg type and the variables. If we could expose in the api not > only the substituted msg but also the variable/value binding, we could > commit to keep the variable names backward compatible. > > So, something like: > > <event href="/ovirt-engine/api/events/8021" id="8021"> > <code>19</code> > <type>USER_ADD_VM_POOL_WITH_VMS_FAILED</type> > <description>the substituted msg.</description> > <parameters> > <parameter> > <key>VmPoolName</key> > <value>The Pool Name<value> > </parameter> > ... > </parameters> > </event> > > Not really rock solid since the variables would still be defined in the > AuditLogMessages.properties but still better and still easier to parse > on the client side. > That makes sense to me. I have opened the following bug to track that: [RFE] Add properties to events https://bugzilla.redhat.com/1456711 <https://bugzilla.redhat.com/1456711> Note that for the particular case of the VM name, which is what ManageIQ is trying to do in that caode, the current best way is to use the <vm .../> link that is part of the event. I have opened the following ManageIQ issue to track it: Avoid parsing the descriptions of events https://github.com/ManageIQ/manageiq-providers-ovirt/issues/45 <https://github.com/ManageIQ/manageiq-providers-ovirt/issues/45> I think it was introduced while working on targeted refresh, but if we have an alternative then of course it would be better.

> > > > > > > > > That means that if we ever change the meaning of a code the ManageIQ > > > provider, for example, will break. > > > > Right,then it indeed needs to stay stable. > > But how is maintaining the enum string different from the code? It is > > the same information, so if MIQ doesn't use the name directly then it > > doesn't really matter if it's a code or string. > > Perhaps deprecate the code and keep the name fixed? > > > > Thanks, > > michal > > > > > > > >>> > > >>> Regards, > > >>> Juan Hernandez > > >>> > > >>> > > >>> _______________________________________________ > > >>> Devel mailing list > > >>> Devel(a)ovirt.org <mailto:Devel@ovirt.org> <mailto:Devel@ovirt.org <mailto:Devel@ovirt.org>> > <mailto:Devel@ovirt.org <mailto:Devel@ovirt.org> <mailto:Devel@ovirt.org <mailto:Devel@ovirt.org>>> > > >>> http://lists.ovirt.org/mailman/listinfo/devel <http://lists.ovirt.org/mailman/listinfo/devel> > <http://lists.ovirt.org/mailman/listinfo/devel <http://lists.ovirt.org/mailman/listinfo/devel>> > > <http://lists.ovirt.org/mailman/listinfo/devel <http://lists.ovirt.org/mailman/listinfo/devel> > <http://lists.ovirt.org/mailman/listinfo/devel <http://lists.ovirt.org/mailman/listinfo/devel>>> > > > > > _______________________________________________ > > Devel mailing list > > Devel(a)ovirt.org <mailto:Devel@ovirt.org> <mailto:Devel@ovirt.org <mailto:Devel@ovirt.org>> > <mailto:Devel@ovirt.org <mailto:Devel@ovirt.org> <mailto:Devel@ovirt.org <mailto:Devel@ovirt.org>>> > > http://lists.ovirt.org/mailman/listinfo/devel <http://lists.ovirt.org/mailman/listinfo/devel> > <http://lists.ovirt.org/mailman/listinfo/devel <http://lists.ovirt.org/mailman/listinfo/devel>> > > <http://lists.ovirt.org/mailman/listinfo/devel <http://lists.ovirt.org/mailman/listinfo/devel> > <http://lists.ovirt.org/mailman/listinfo/devel <http://lists.ovirt.org/mailman/listinfo/devel>>> > > > > > > _______________________________________________ Devel mailing list Devel(a)ovirt.org <mailto:Devel@ovirt.org> http://lists.ovirt.org/mailman/listinfo/devel <http://lists.ovirt.org/mailman/listinfo/devel>