Clean up & deprecate crashdump #3049
Conversation
There is no crashdump.create API call, so it's fine to remove this function. Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>
Use an equivalent one from Helpers instead. Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>
|
|
||
| let nothrow f () = try f() with _ -> () | ||
|
|
||
| let create ~__context ~vM ~vDI = |
There was a problem hiding this comment.
Should we leave the create function here in case it's needed in the future?
There was a problem hiding this comment.
They can still find it in the git history though, and @jonludlam said that this API class isn't really used anyway.
There was a problem hiding this comment.
I've removed it because I would have to add a check to it, but realized it's unused, removing these function saves use time I think, because we don't have to consider them when we make changes.
| Db.Crashdump.create ~__context ~ref:cdumpref ~uuid ~vM ~vDI ~other_config:[]; | ||
| cdumpref | ||
|
|
||
| let destroy ~__context ~self = |
There was a problem hiding this comment.
Where do we use this? Can't we just move the function in an helper module and get rid of this module altogether?
There was a problem hiding this comment.
This is a XenAPI call, crashdump.destroy: https://xapi-project.github.io/xen-api/classes/crashdump.html,
so I think it's best to stick to the existing conventions and keep this file.
There was a problem hiding this comment.
Then let's deprecate the whole class! 🗑️
Also have a look at the use of host_crashdump but I think that cannot be deprecated yet.
There was a problem hiding this comment.
Yep, but I think we can still merge this PR, as we can't just delete the whole class and its associated code right away, but this is still a useful cleanup in the meantime?
There was a problem hiding this comment.
Please add the deprecation of the Crashdump class to this PR.
There was a problem hiding this comment.
Sounds good, I'll add that!:)
There was a problem hiding this comment.
@mseri I think we first need to add the next release (inverness?) to the datamodel, before we can deprecate this class?
gaborigloi
force-pushed
the
xapi_crashdump_cleanup
branch
from
6ca1f43 to
3c23886
Compare
| version_minor = 7; | ||
| branding = "XenServer 7.2"; | ||
| }; { | ||
| code_name = Some rel_inverness; |
There was a problem hiding this comment.
What happens if you put None here?
There was a problem hiding this comment.
I think None here is for things that do not have a codename, but Inverness does.
There was a problem hiding this comment.
Another option is to simply not add Inverness to release_order_full yet, and add it later. But maybe that would confuse the SDK or something.
There was a problem hiding this comment.
Please leave the same version numbers as falcon and use Unreleased for the branding. We'll bump it when we know the actual number.
This is basically the same we were doing before, it's just that now all these variables are in one place.
There was a problem hiding this comment.
Actually it's probably worth adding a comment at the beginning of this list with this tip for future reference.
There was a problem hiding this comment.
@kc284 How about a comment like this:
(* For future XenServer versions, use the version number of the latest release, and "Unreleased" for the branding. *)
There was a problem hiding this comment.
I'd rather make it more explicit and less xenserver-centric: "When you add a new release, use .... until the actual values are finalised".
Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>
Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>
Signed-off-by: Gabor Igloi <gabor.igloi@citrix.com>
gaborigloi
force-pushed
the
xapi_crashdump_cleanup
branch
from
3c23886 to
caa15cd
Compare
crashdumpXenAPI class