feat: Added Semver Version and Versionreq support for plugin versions #942
Conversation
devin-b-lake
force-pushed
the
plugin-semver-version-update
branch
from
8295e71 to
d50cc25
Compare
|
The |
|
The policy-file.md documentation file where I updated the plugin version information includes the line 'For an example of the manifest field, see I looked for the 'For-Developers' section and references to |
Sure Devin that would be helpful. |
| // If exact plugin version was provided, use existing cache entry if not force | ||
| if plugin_id | ||
| .version() | ||
| .version_req | ||
| .comparators | ||
| .first() | ||
| .is_some_and(|comp| comp.op == semver::Op::Exact) | ||
| { | ||
| let version_req_syntax_string = plugin_id.version().version_req.to_string(); | ||
| let version_syntax_string = version_req_syntax_string | ||
| .chars() | ||
| .skip(1) | ||
| .collect::<String>(); | ||
| let plugin_version = PluginVersion::new(version_syntax_string.as_str())?; | ||
| let plugin_id_for_cache = PluginId::new( | ||
| plugin_id.publisher().clone(), |
There was a problem hiding this comment.
Really clean, I like this. Might want to check though if this code stands up to a version req like =0.3.1,>4.0.1. If VersionReq::parse doesn't catch a string like that as being nonsense, the code here might not get a proper concrete version number
There was a problem hiding this comment.
If a nonsense version req is passed in, e.g., =0.3.1, >4.0.1, how do we want to handle it? Should it error or should we assign a version? If we assign a version, should we make it the latest available plugin version?
It looks like the SemVer VersionReq doesn't detect =0.3.1,>4.0.1 is nonsense; it creates a VersionReq object with that version range. I'll have to rethink this logic.
There was a problem hiding this comment.
Nonsense reqs should be rejected.
There was a problem hiding this comment.
Copy that thanks. Currently, the program errors when a nonsense version req is passed into the version field because it cannot find a version in the download manifest within the nonsense range.
If we want the nonsense version req to be rejected when the PluginVersionReq object is instantiated (PluginVersionReq is a wrapper for VersionReq), I can try to write a validation function into the PluginVersionReq struct.
Which approach should we go with?
There was a problem hiding this comment.
I think erroring as early as possible is better, and means we can give clear error messages when a constraint is not satisfiable inherently because it's expressing incompatible bounds
There was a problem hiding this comment.
Sorry, the point of my original comment was not to get into discussion about how to detect nonsense versions. The point is that a version req could have multiple parts, in which case if the first comparator is =, we can't necessarily assume that the rest of the string can be parsed as a Version.
There was a problem hiding this comment.
For example, if I do this:
plugins {
plugin "mitre/activity" version="=0.4.0,>0.2.0" manifest="https://hipcheck.mitre.org/dl/plugin/mitre/activity.kdl"
}
Hipcheck errors with: unexpected character ',' after patch version number
There was a problem hiding this comment.
Thanks for all the work on this @devin-b-lake! I added my comments under points Julian had made already. I'd say the biggest thing is to be clear in our documentation and naming in the code itself where we're dealing with versions (as specified by SemVer) vs. version requirements.
Signed-off-by: Devin Lake <[email protected]>
devin-b-lake
force-pushed
the
plugin-semver-version-update
branch
from
e38c5c0 to
53d85ad
Compare
No description provided.