Move bwcVersions extension property to BuildParams #56206
Conversation
breskeby
added
>enhancement
:Delivery/Build
|
Pinging @elastic/es-core-infra (:Core/Infra/Build) |
| List<String> versionLines = IOUtils.readLines(new FileInputStream(versionsFile), "UTF-8"); | ||
| return new BwcVersions(versionLines); | ||
| } catch (IOException e) { | ||
| return null; |
There was a problem hiding this comment.
I think we should rethrow as an UncheckedIOException. If we aren't able to determine BWC versions this is likely to cause all sorts of build issues so we should treat this as a fatal error. I believe this might already be the case as BuildParams will validate we aren't setting the value to null, but then we end up losing the original exception cause which is not ideal.
There was a problem hiding this comment.
I thought about it but didn’t want to fail fatal here to make testing easier. I needed to touch the testkit Test builds and wanted to keep the ability to direct configure the bwcVersion in via BuildParamd there instead of introducing sth like a test only override property in the plugin itself to pass a configurable file path to resolve versions from
There was a problem hiding this comment.
I thought about it but didn’t want to fail fatal here to make testing easier. I needed to touch the testkit Test builds and wanted to keep the ability to direct configure the bwcVersion in via BuildParamd there instead of introducing sth like a test only override property in the plugin itself to pass a configurable file path to resolve versions from
There was a problem hiding this comment.
Gotcha, we should at least log the original exception then, so if it does barf we get a better message than just bwcVersions cannot be null or whatever.
There was a problem hiding this comment.
Why does not failing with an exception make this easier to test? Tests can use expectThrows to catch an exception?
There was a problem hiding this comment.
That doesn't work with testKit tests since it's not the test that's throwing the exception, it's the build and those don't get propagated to the test. You just get a "your build failed" type exception returned by teskit.
|
I thought about it but didn’t want to fail fatal here to make testing easier. I needed to touch the testkit Test builds and wanted to keep the ability to direct configure the bwcVersion in via BuildParamd there instead of introducing sth like a test only override property in the plugin itself to pass a configurable file path to resolve versions from
… On 5. May 2020, at 18:50, Mark Vieira ***@***.***> wrote:
@mark-vieira approved this pull request.
One comment on error handling but otherwise LGTM.
In buildSrc/src/main/java/org/elasticsearch/gradle/info/GlobalBuildInfoPlugin.java:
> });
// Print global build info header just before task execution
project.getGradle().getTaskGraph().whenReady(graph -> logGlobalBuildInfo());
}
+ /* Introspect all versions of ES that may be tested against for backwards
+ * compatibility. It is *super* important that this logic is the same as the
+ * logic in VersionUtils.java. */
+ private static BwcVersions resolveBwcVersions(File root) {
+ File versionsFile = new File(root, DEFAULT_VERSION_JAVA_FILE_PATH);
+ try {
+ List<String> versionLines = IOUtils.readLines(new FileInputStream(versionsFile), "UTF-8");
+ return new BwcVersions(versionLines);
+ } catch (IOException e) {
+ return null;
I think we should rethrow as an UncheckedIOException. If we aren't able to determine BWC versions this is likely to cause all sorts of build issues so we should treat this as a fatal error. I believe this might already be the case as BuildParams will validate we aren't setting the value to null, but then we end up losing the original exception cause which is not ideal.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
The problem is not about testing the exception handling but tweak testing scenarios when e.g. testing the DistributionDownloadPlugin by injecting custom versions
… On 6. May 2020, at 00:30, Ryan Ernst ***@***.***> wrote:
@rjernst commented on this pull request.
In buildSrc/src/main/java/org/elasticsearch/gradle/info/GlobalBuildInfoPlugin.java:
> });
// Print global build info header just before task execution
project.getGradle().getTaskGraph().whenReady(graph -> logGlobalBuildInfo());
}
+ /* Introspect all versions of ES that may be tested against for backwards
+ * compatibility. It is *super* important that this logic is the same as the
+ * logic in VersionUtils.java. */
+ private static BwcVersions resolveBwcVersions(File root) {
+ File versionsFile = new File(root, DEFAULT_VERSION_JAVA_FILE_PATH);
+ try {
+ List<String> versionLines = IOUtils.readLines(new FileInputStream(versionsFile), "UTF-8");
+ return new BwcVersions(versionLines);
+ } catch (IOException e) {
+ return null;
Why does not failing with an exception make this easier to test? Tests can use expectThrows to catch an exception?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Yeah, since this is now done by a plugin instead of the by root build script those plugin tests include this logic so the only other way to override would be to create a That brings up a good point though. Will external users of I think we have a couple options here:
|
We have a guard on this already. In all the places we use bwcVersions, we check isInternal. This is something we should fix, either by having a buildSrc build time copy of Version.java into buildSrc resources, or read Version.java at that time and generating a json resource file that can be loaded more directly, but that we don't need to worry about bad formatting or anything missing (my preferred approach). |
What would be the benefit of this if we never intend external users to need BWC information? |
|
The bwc information is needed to run any kind of bwc tests. It's reasonable to assume external plugin authors will want to test compatibility of their own plugin against other versions when we finally break lockstep plugin versioning. |
|
Sure, but right now we have no kind of guarantees on how this works, and there exists no documentation or guidance on how one might integration BWC testing into their plugin build. I'd say until we officially support such a thing we make bwcversions a purely internal mechanism (guarded like it is as you mention) and simply blow up if you try to access it outside the Elasticsearch build. |
|
That's fair, but don't external plugin authors still load GlobalBuildInfoPlugin? How can we blow up in trying to load it outside of ES without causing all plugin authors builds to fail? |
|
We do as Rene has done here and gracefully handle a missing This behavior is essentially no different than trying to read |
|
Can't we use |
Good call. @rene, let's do what @rjernst suggests. If we detect that this is an "internal" build (meaning we are running in the elasticsearch project vs the |
|
Will do that check on isInternal. Good idea and thanks for the discussion here.
… On 6. May 2020, at 04:33, Mark Vieira ***@***.***> wrote:
Can't we use isInternal to determine whether we should fail?
Good call. @rene, let's do what @rjernst suggests. If we detect that this is an "internal" build (meaning we are running in the elasticsearch project vs the build-tools jar being pulled in by an external project) and we fail to parse Version.java for any reason let's explode, otherwise return null as we do now and then fail on read.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
I tweaked the implementation to only set the BuildParams.bwcVersions property for internal builds and fail early when 1. sth goes wrong in parsing the version file and 2. if a passed bwcVersion to BuildParams.setBwcVersion(...) is null. IMO this is the most robust approach. |
|
@elasticmachine update branch |
|
@elasticmachine test this please |
|
@elasticmachine update branch |
|
@elasticmachine test this please |
| BuildParams.init(params -> { | ||
| // Initialize global build parameters | ||
| boolean internalUse = GlobalBuildInfoPlugin.class.getResource("/buildSrc.marker") != null; |
There was a problem hiding this comment.
nit: I would name this isInternal since that is the property we are setting
|
Changes LGTM. |
- resolved in GlobalBuildInfoPlugin - TODO: probably more bwc related work can be moved from build scripts into certain plugins.
- propagate exception if version file cannot be resolved - add null check when setting bwcVersions
I think we missed this spot in elastic#56206 breaking the task and thus Gradle imports into Idea.
I think we missed this spot in #56206 breaking the task and thus Gradle imports into Idea.
|
@breskeby can you please ensure this change is backported to |
- resolved in GlobalBuildInfoPlugin - propagate exception if version file cannot be resolved - add null check when setting bwcVersions
I think we missed this spot in elastic#56206 breaking the task and thus Gradle imports into Idea.
|
7.x Backport PR: #56381 |
- resolved in GlobalBuildInfoPlugin - propagate exception if version file cannot be resolved - add null check when setting bwcVersions
I think we missed this spot in elastic#56206 breaking the task and thus Gradle imports into Idea.
- resolved in GlobalBuildInfoPlugin - propagate exception if version file cannot be resolved - add null check when setting bwcVersions
I think we missed this spot in elastic#56206 breaking the task and thus Gradle imports into Idea.
|
7.8 backport PR #56382 |
|
7.7 backport PR #56383 |
* Move bwcVersions extension property to BuildParams (#56206) * Fix :qa Task Using Broken BwC Versions Resolution (#56332) Co-authored-by: Armin Braun <[email protected]>
* Move bwcVersions extension property to BuildParams (#56206) * Fix :qa Task Using Broken BwC Versions Resolution (#56332) Co-authored-by: Armin Braun <[email protected]>
* Move bwcVersions extension property to BuildParams (#56206) * Fix :qa Task Using Broken BwC Versions Resolution (#56332) Co-authored-by: Armin Braun <[email protected]>
mark-vieira
added
Team:Delivery
certain plugins.