-
Notifications
You must be signed in to change notification settings - Fork 25.5k
Add and modify mappings between version ids and release versions #103627
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
...ols-internal/src/main/java/org/elasticsearch/gradle/internal/release/ReleaseToolsPlugin.java
Outdated
Show resolved
Hide resolved
773ae83
to
4df06c3
Compare
import javax.annotation.Nullable; | ||
import javax.inject.Inject; | ||
|
||
public class TagVersionsTask extends DefaultTask { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing something? I don't see where this task is actually used/created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this needs corresponding changes in release-tools (this PR is still WIP)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created a PR for the release tools to use these new tasks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a nice addition to give users back versions they know about! I have a few thoughts.
...-tools-internal/src/main/java/org/elasticsearch/gradle/internal/release/TagVersionsTask.java
Outdated
Show resolved
Hide resolved
...-tools-internal/src/main/java/org/elasticsearch/gradle/internal/release/TagVersionsTask.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
public static String findReleaseVersion(Class<?> versionContainer, int id) { | ||
if (USES_VERSIONS == false) return Integer.toString(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means in serverless we would print the raw id. Is that what we want? The id is just as useless to a user there. Are we assuming a serverless user should never hit an error that would need to print the release version since it is kept up to date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly aimed at log messages - where transport/index versions do have meaning in serverless. If we ever need to print out anything for serverless, it should be the raw id, not the release version.
|
||
@Override | ||
public void accept(FieldDeclaration fieldDeclaration) { | ||
var ints = fieldDeclaration.findAll(IntegerLiteralExpr.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fragile, wouldn't any other integer literal get caught up by this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do only have single int initializers in version classes. And it ignores those that have multiple ints in the initializer.
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Hi @thecoop, I've created a changelog YAML for you. |
Hi @thecoop, I've created a changelog YAML for you. |
...rnal/src/main/java/org/elasticsearch/gradle/internal/release/ExtractCurrentVersionsTask.java
Outdated
Show resolved
Hide resolved
@elasticmachine update branch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, a couple last thoughts, and Mark's comment on write options should be addressed as well.
server/src/test/java/org/elasticsearch/ReleaseVersionsTests.java
Outdated
Show resolved
Hide resolved
add 8.12 version numbers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, and with the helpers in the XxxVersions classes it will be easy to use too.
I'm wondering if we should have something similar for features too - probably worth a separate discussion (and definitely a separate PR in case).
server/src/test/java/org/elasticsearch/ReleaseVersionsTests.java
Outdated
Show resolved
Hide resolved
`ReleaseVersions` does some nontrivial initialization. It shouldn't fail, but if it does then it should do so early in startup rather than waiting until the class is first used, which may be in a context in which failure is undesirable. This commit adds it to the list of classes that are initialized even before the security manager is installed. Relates elastic#103627
`ReleaseVersions` does some nontrivial initialization. It shouldn't fail, but if it does then it should do so early in startup rather than waiting until the class is first used, which may be in a context in which failure is undesirable. This commit adds it to the list of classes that are initialized even before the security manager is installed. Relates #103627
Add a
ReleaseVersions
class to map between ids and release versions in error messages/logs, and gradle tasks to extract current version information and apply it to the version records in the repo