Use string version internally for PluginDescriptor #100713
Conversation
williamrandolph
added
>non-issue
:Core/Infra/Core
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
| Version.writeVersion(Version.fromString(elasticsearchVersion), out); | ||
| } catch (IllegalArgumentException e) { | ||
| // can't parse current version, so send placeholder | ||
| Version.writeVersion(BWC_PLACEHOLDER_VERSION, out); |
There was a problem hiding this comment.
I like this solution; I was contemplating of doing something similar myself (we are solving basically the same problem in 2 places - #100746 - but I was wondering what version could be used. I was thinking Version.CURRENT. What made you choose a fixed one?
(I will update my PR to be consistent with yours afterwards)
There was a problem hiding this comment.
I would use Version.current if possible, is there a reason it can't work?
There was a problem hiding this comment.
Is Version.CURRENT going to stick around indefinitely? I went with a placeholder because I wasn't sure if we were going to get rid of Version.CURRENT altogether at some point.
There was a problem hiding this comment.
We do want to get rid of it. Perhaps we can defer what to do here because this still doesn't seem like a case we need to catch/send a fallback, as noted in my other comment on this block.
| @@ -163,7 +163,7 @@ private static NodeInfo createNodeInfo() { | |||
| randomAlphaOfLengthBetween(3, 10), | |||
| randomAlphaOfLengthBetween(3, 10), | |||
| randomAlphaOfLengthBetween(3, 10), | |||
| VersionUtils.randomVersion(random()), | |||
| randomAlphaOfLengthBetween(3, 10), | |||
There was a problem hiding this comment.
Just a note for the record: I am using randomAlphaOfLengthBetween(6, 32) as a replacement for version (to cover "8.12.0" as well as a build hash). But it's just a note: I think every random "any string" have to work.
| Version.writeVersion(Version.fromString(elasticsearchVersion), out); | ||
| } catch (IllegalArgumentException e) { | ||
| // can't parse current version, so send placeholder | ||
| Version.writeVersion(BWC_PLACEHOLDER_VERSION, out); |
There was a problem hiding this comment.
I would use Version.current if possible, is there a reason it can't work?
server/src/main/java/org/elasticsearch/plugins/PluginDescriptor.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/plugins/PluginDescriptor.java
Outdated
Show resolved
Hide resolved
| @@ -106,6 +108,9 @@ public PluginDescriptor( | |||
| this.isLicensed = isLicensed; | |||
| this.isModular = isModular; | |||
| this.isStable = isStable; | |||
| this.builtWithSemanticVersion = Optional.ofNullable(elasticsearchVersion) | |||
There was a problem hiding this comment.
I get what you are trying to do, but I don't think this is something we need to worry about right now. Plugins cannot be built outside of an ES "semantic version", even for serverless. I think we should change this simply here for now (just assume a semantic version string), and figure out the best way to represent serverless plugins later when we are ready for that.
There was a problem hiding this comment.
It looks to me like PluginUtils#verifyCompatiblity gets called even for the built-in module case, though. If the version field in the plugin descriptor starts getting built with an commit hash value (rather than a 8.x.y-SNAPSHOT value), it seems like validation will fail at that point.
Should I instead find a way to skip that verification for modules in serverless, or can I assume that serverless's plugin building code will always write a semantic version to the elasticsearch.version field of the plugin descriptor?
There was a problem hiding this comment.
I don't know about "always", but for the time being, the build won't change (we will continue to have the semantic version in the build). Note this isn't just plugin metadata, we have the semantic version in the file paths themselves. This is ok since it is completely local to a node; we don't actually care what the filenames are.
There was a problem hiding this comment.
I've removed the code path checking for non-semantic versions.
Since the work to use a String version in the plugin descriptor is pretty simple, I'm putting it in its own PR and saving the work changing
PluginDescriptor#getElasticsearchVersionto return a string for a follow-up PR.