[test] packaging: add windows boxes #30402

andyb-elastic · 2018-05-04T21:37:28Z

Adds windows server 2012r2 and 2016 vagrant boxes to packaging tests.
They can only be used if IDs for their images are specified, which are
passed to gradle and then to vagrant via env variables. Adds options
to the project property vagrant.boxes to choose between linux and
windows boxes

Bats tests are run only on linux boxes, and portable packaging tests run
on all boxes. Platform tests are only run on linux boxes since they are
not being maintained.

For #26741

The full packaging test jobs (both periodic and pull request) should be all set up to use these boxes once this is merged.

elasticmachine · 2018-05-04T21:37:30Z

Pinging @elastic/es-core-infra

Adds windows server 2012r2 and 2016 vagrant boxes to packaging tests. They can only be used if IDs for their images are specified, which are passed to gradle and then to vagrant via env variables. Adds options to the project property `vagrant.boxes` to choose between linux and windows boxes Bats tests are run only on linux boxes, and portable packaging tests run on all boxes. Platform tests are only run on linux boxes since they are not being maintained. For elastic#26741

nik9000

I left two questions but most of it makes sense to me.

nik9000 · 2018-05-10T17:34:28Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/vagrant/VagrantTestPlugin.groovy

@@ -59,8 +68,10 @@ class VagrantTestPlugin implements Plugin<Project> {
    @Override
    void apply(Project project) {

+        collectAvailableBoxes(project)


Since this is static maybe you should do it in a static block instead of on plugin application.

Sure that makes sense. Actually general question about that (static block vs static method). I've always erred on the side of not doing stuff in static blocks when it could conceivably blow up (not sure if that applies here) because it seems more intuitive for it to blow up when it's used (in this case when the plugin is applied) than when the class is loaded. Does that make sense or is that just a weird preconception I have

It makes some sense, yeah. static blocks are executed when the class is loaded but something like this is executed on every plugin apply. Minimally, the stack trace of the second one is going to be more useful than the first. You could certainly do the static stuff on first plugin apply, but that feels a little more complicated than it is worth in this scenario.

That's a good point. I'll put it in a static block since it's applicable to all applications of the plugin, in contrast to some of the other configuration here which uses properties of the project

This method is now non-static because of some changes I made to avoid separate plugin instances stomping on each others' configuration

nik9000 · 2018-05-10T17:36:29Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/vagrant/VagrantTestPlugin.groovy

+                'VAGRANT_VAGRANTFILE': 'Vagrantfile',
+                'VAGRANT_PROJECT_DIR': "${project.projectDir.absolutePath}"
+        ]
+        vagrantEnvVars.putAll(VAGRANT_ENV_VARS)


I think it is weird to have these named the same way. Maybe look in WINDOWS_BOXES instead? Or name VAGRANT_ENV_VARS to VAGRANT_BOX_ENV_VARS or something.

I agree, I'll make the naming more clear.

Actually originally I wrote it like

def vagrantEnvVars = [ ... ] if ('windows-2016' in WINDOWS_BOXES) { vagrantEnvVars.put('VAGRANT_WINDOWS_2016_BOX', WINDOWS_BOXES['windows-2016']) } // same for windows 2012r2 return vagrantEnvVars

but changed it to just store the env vars directly because it seemed like an unnecessary level of indirection. What do you think?

I'm fine with digging through the WINDOWS_BOXES though I might want add something that fails if there is an unknown key in there. It'd be inconvenient to just add something to that Map and forget to update this method and have the box fail to show up mysteriously.

Gotcha, I kind of feel like getting them from WINDOWS_BOXES has more places where it could go wrong than it needs to. I'll change the names to make it more clear what's happening

Get windows box IDs from environment variables in static context instead of on plugin application Clarify how the plugin determines the environment variables to run vagrant with

andyb-elastic · 2018-05-10T21:06:25Z

@nik9000 I pushed 0a5b7dd

I got rid of the method that built the vagrant env vars and put that section back where it was - that method was also left over from another implementation

rjernst

Looks great @andyb-elastic! I left a few comments.

rjernst · 2018-05-10T21:17:01Z

TESTING.asciidoc

+* `all` - All configured boxes, i.e. all linux boxes and any configured Windows
+boxes.
+* `linux-all` - All linux boxes.
+* `windows-all` - All configured Windows boxes. If there are none configured when this


Thank you for avoiding leniency!

Out of curiosity, do we fail if an empty string is passed to vagrant.boxes? (separate issue if we don't)

It does fail with an empty string, because it ends up looking for a box with the name of empty string and can't find it

rjernst · 2018-05-10T21:20:21Z

TESTING.asciidoc

+These variables are required for Windows support in all gradle tasks that
+handle packaging tests. Either or both may be specified. Remember that to run tests
+on these boxes, they still need to be included in the value of `-Pvagrant.boxes`.
+When these properties are present, passing `-Pvagrant.boxes=all` will include the


Maybe this should fail if they are not configured? Users can run with linux-all to get the old behavior.

rjernst · 2018-05-10T21:22:06Z

TESTING.asciidoc


-* Windows Server 2012
+* `VAGRANT_WINDOWS_2012R2_BOX`


I think gradle properties might be better to use here. That way we could set them in .gradle/gradle.properties, or configure them via another gradle project within the multi project build in elasticsearch-extra. These could be vagrant.windows-2012r2.id and vagrant.windows-2016.id. They can still be passed into vagrant via env vars, but from the gradle perspective I think this would be better.

Note that to allow them to be set by a sibling project, we need to load them lazily (execution time).

rjernst · 2018-05-10T21:33:00Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/vagrant/VagrantTestPlugin.groovy

-                environmentVars vagrantEnvVars
-                dependsOn up, setupPackagingTest
-                finalizedBy halt
+            if (box in LINUX_BOXES) {


Please use a LINUX_BOXES.contains(box) instead of groovy's in operator, so we stay as close to java as possible (we would like to eventually switch buildSrc to all java, for performance reasons, so keeping the code as java like as possible will help with that transition). Ditto below for the other similar conditions.

Set windows images ids from gradle project properties instead of environment variables Make vagrant test plugin configuration nonstatic so applications of the plugin don't stomp each other Make the plugin aware of which boxes make up the universe of ones that we test, versus what images are actually available from the configuration at runtime, and fail if those don't match up. Add tasks to list all boxes and available boxes so it's clear to the user what configuration gradle is seeing.

andyb-elastic · 2018-05-11T22:10:25Z

@rjernst I pushed f8d3db2 to address your comments

The main thing I changed was to make it keep track separately of all the boxes we test, and which ones have images available, so now it complains if any are missing. That is, it's strict in that both vagrant.boxes=all and vagrant.boxes=windows-all will fail the build if there are any windows images missing.

Note that the packaging CI job will fail for these for that reason until the workers' gradle.properties have the windows image names added, which I'll open a PR for

rjernst

LGTM, one final comment.

rjernst · 2018-05-12T04:33:10Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/vagrant/VagrantTestPlugin.groovy

+    ])
+
+    /** Boxes that have been supplied and are available for testing **/
+    List<String> AVAILABLE_BOXES = []


This and the env vars should be camelCase since they are not final.

Can you put the non-final stuff below the final stuff? Mixing them up bothers me more than it should.

Agree, makes it much more clear how it works

nik9000 · 2018-05-14T16:11:28Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/vagrant/VagrantTestPlugin.groovy

+    ])
+
+    /** Boxes that have been supplied and are available for testing **/
+    List<String> AVAILABLE_BOXES = []


Can you put the non-final stuff below the final stuff? Mixing them up bothers me more than it should.

Rename and relocate nonfinal, nonstatic box list members

andyb-elastic · 2018-05-15T00:33:20Z

Jenkins please run gradle build tests and please run all packaging tests

andyb-elastic · 2018-05-15T03:46:12Z

Jenkins run gradle build tests please

andyb-elastic · 2018-05-15T17:41:05Z

Jenkins run gradle build tests please

andyb-elastic · 2018-05-15T19:53:31Z

Jenkins yet again run gradle build tests please

andyb-elastic · 2018-05-15T21:20:41Z

Jenkins run gradle build tests please

andyb-elastic · 2018-05-16T00:20:26Z

Jenkins run gradle build tests please

andyb-elastic · 2018-05-16T17:31:55Z

Jenkins run gradle build tests please

* es/master: (74 commits) Preserve REST client auth despite 401 response (#30558) [test] packaging: add windows boxes (#30402) Make xpack modules instead of a meta plugin (#30589) Mute ShrinkIndexIT [ML] DeleteExpiredDataAction should use client with origin (#30646) Reindex: Fixed typo in assertion failure message (#30619) [DOCS] Fixes list of unconverted snippets in build.gradle [DOCS] Reorganizes RBAC documentation SQL: Remove dependency for server's version from JDBC driver (#30631) Test: increase search logging for LicensingTests Adjust serialization version in IndicesOptions [TEST] Fix compilation Remove version argument in RangeFieldType (#30411) Remove unused DirectoryUtils class. (#30582) Mitigate date histogram slowdowns with non-fixed timezones. (#30534) Add a MovingFunction pipeline aggregation, deprecate MovingAvg agg (#29594) Removes AwaitsFix on IndicesOptionsTests Template upgrades should happen in a system context (#30621) Fix bug in BucketMetrics path traversal (#30632) Fixes IndiceOptionsTests to serialise correctly (#30644) ...

* es/ccr: (75 commits) Preserve REST client auth despite 401 response (elastic#30558) [test] packaging: add windows boxes (elastic#30402) Make xpack modules instead of a meta plugin (elastic#30589) Mute ShrinkIndexIT [ML] DeleteExpiredDataAction should use client with origin (elastic#30646) Reindex: Fixed typo in assertion failure message (elastic#30619) [DOCS] Fixes list of unconverted snippets in build.gradle [DOCS] Reorganizes RBAC documentation SQL: Remove dependency for server's version from JDBC driver (elastic#30631) Test: increase search logging for LicensingTests Adjust serialization version in IndicesOptions [TEST] Fix compilation Remove version argument in RangeFieldType (elastic#30411) Remove unused DirectoryUtils class. (elastic#30582) Mitigate date histogram slowdowns with non-fixed timezones. (elastic#30534) Add a MovingFunction pipeline aggregation, deprecate MovingAvg agg (elastic#29594) Removes AwaitsFix on IndicesOptionsTests Template upgrades should happen in a system context (elastic#30621) Fix bug in BucketMetrics path traversal (elastic#30632) Fixes IndiceOptionsTests to serialise correctly (elastic#30644) ...

Adds windows server 2012r2 and 2016 vagrant boxes to packaging tests. They can only be used if IDs for their images are specified, which are passed to gradle and then to vagrant via env variables. Adds options to the project property `vagrant.boxes` to choose between linux and windows boxes. Bats tests are run only on linux boxes, and portable packaging tests run on all boxes. Platform tests are only run on linux boxes since they are not being maintained. For #26741

…ngs-to-true * elastic/master: (25 commits) [DOCS] Replace X-Pack terms with attributes [ML] Clean left behind model state docs (elastic#30659) Correct typos filters agg docs duplicated 'bucket' word removal (elastic#30677) top_hits doc example description update (elastic#30676) [Docs] Replace InetSocketTransportAddress with TransportAdress (elastic#30673) [TEST] Account for increase in ML C++ memory usage (elastic#30675) User proper write-once semantics for GCS repository (elastic#30438) Remove bogus file accidentally added Add detailed assert message to IndexAuditUpgradeIT (elastic#30669) Adjust fast forward for token expiration test (elastic#30668) Improve explanation in rescore (elastic#30629) Deprecate `nGram` and `edgeNGram` names for ngram filters (elastic#30209) Watcher: Fix watch history template for dynamic slack attachments (elastic#30172) Fix _cluster/state to always return cluster_uuid (elastic#30656) [Tests] Add debug information to CorruptedFileIT Preserve REST client auth despite 401 response (elastic#30558) [test] packaging: add windows boxes (elastic#30402) Make xpack modules instead of a meta plugin (elastic#30589) Mute ShrinkIndexIT ...

* 6.x: Mute testCorruptFileThenSnapshotAndRestore Plugins: Remove meta plugins (#30670) Upgrade to Lucene-7.4.0-snapshot-59f2b7aec2 (#30726) Docs: Add uptasticsearch to list of clients (#30738) [TEST] Reduce forecast overflow to disk test memory limit (#30727) [DOCS] Removes redundant index.asciidoc files (#30707) [DOCS] Moves X-Pack configurationg pages in table of contents (#30702) [ML][TEST] Fix bucket count assertion in ModelPlotsIT (#30717) [ML][TEST] Make AutodetectMemoryLimitIT less fragile (#30716) [Build] Add test admin when starting gradle run with trial license and [ML] provide tmp storage for forecasting and possibly any ml native jobs #30399 Tests: Fail if test watches could not be triggered (#30392) Watcher: Prevent duplicate watch triggering during upgrade (#30643) [ML] add version information in case of crash of native ML process (#30674) Add detailed assert message to IndexAuditUpgradeIT (#30669) Preserve REST client auth despite 401 response (#30558) Make TransportClusterStateAction abide to our style (#30697) [DOCS] Fixes edit URLs for stack overview (#30583) [DOCS] Add missing callout in IndicesClientDocumentationIT Backport get settings API changes to 6.x (#30494) Silence sleep based watcher test [DOCS] Replace X-Pack terms with attributes Improve explanation in rescore (#30629) [test] packaging: add windows boxes (#30402) [ML] Clean left behind model state docs (#30659) filters agg docs duplicated 'bucket' word removal (#30677) top_hits doc example description update (#30676) MovingFunction Pipeline agg backport to 6.x (#30658) [Docs] Replace InetSocketTransportAddress with TransportAdress (#30673) [TEST] Account for increase in ML C++ memory usage (#30675) User proper write-once semantics for GCS repository (#30438) Deprecate `nGram` and `edgeNGram` names for ngram filters (#30209) Watcher: Fix watch history template for dynamic slack attachments (#30172) Fix _cluster/state to always return cluster_uuid (#30656)

Adds windows server 2012r2 and 2016 vagrant boxes to packaging tests. They can only be used if IDs for their images are specified, which are passed to gradle and then to vagrant via env variables. Adds options to the project property `vagrant.boxes` to choose between linux and windows boxes. Bats tests are run only on linux boxes, and portable packaging tests run on all boxes. Platform tests are only run on linux boxes since they are not being maintained. For elastic#26741

andyb-elastic added >test Issues or PRs that are addressing/adding tests :Delivery/Packaging RPM and deb packaging, tar and zip archives, shell and batch scripts v7.0.0 v6.4.0 labels May 4, 2018

andyb-elastic requested a review from rjernst May 4, 2018 21:37

andyb-elastic force-pushed the feature-packaging-windows-boxes branch from ecef367 to d32c16a Compare May 7, 2018 20:30

jasontedor requested a review from nik9000 May 9, 2018 00:47

nik9000 reviewed May 10, 2018

View reviewed changes

code review fixes

0a5b7dd

Get windows box IDs from environment variables in static context instead of on plugin application Clarify how the plugin determines the environment variables to run vagrant with

rjernst reviewed May 10, 2018

View reviewed changes

rjernst approved these changes May 12, 2018

View reviewed changes

nik9000 approved these changes May 14, 2018

View reviewed changes

code review fixes

d7f2e71

Rename and relocate nonfinal, nonstatic box list members

Merge branch 'master' into feature-packaging-windows-boxes

f124fb2

Merge branch 'master' into feature-packaging-windows-boxes

0723ed7

Merge branch 'master' into feature-packaging-windows-boxes

3d9c611

andyb-elastic merged commit 890afad into elastic:master May 16, 2018

andyb-elastic mentioned this pull request May 16, 2018

[test] packaging: add windows boxes backport 6.x #30664

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test] packaging: add windows boxes #30402

[test] packaging: add windows boxes #30402

andyb-elastic commented May 4, 2018

elasticmachine commented May 4, 2018

nik9000 left a comment

nik9000 May 10, 2018

andyb-elastic May 10, 2018

nik9000 May 10, 2018

andyb-elastic May 10, 2018

andyb-elastic May 11, 2018

nik9000 May 10, 2018

andyb-elastic May 10, 2018

nik9000 May 10, 2018

andyb-elastic May 10, 2018

andyb-elastic commented May 10, 2018

rjernst left a comment

rjernst May 10, 2018

rjernst May 10, 2018

andyb-elastic May 10, 2018

rjernst May 10, 2018

rjernst May 10, 2018

rjernst May 10, 2018

rjernst May 10, 2018

andyb-elastic commented May 11, 2018

rjernst left a comment

rjernst May 12, 2018

nik9000 May 14, 2018

andyb-elastic May 14, 2018

nik9000 May 14, 2018

andyb-elastic commented May 15, 2018

andyb-elastic commented May 15, 2018

andyb-elastic commented May 15, 2018

andyb-elastic commented May 15, 2018

andyb-elastic commented May 15, 2018

andyb-elastic commented May 16, 2018

andyb-elastic commented May 16, 2018

[test] packaging: add windows boxes #30402

[test] packaging: add windows boxes #30402

Conversation

andyb-elastic commented May 4, 2018

elasticmachine commented May 4, 2018

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyb-elastic commented May 10, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyb-elastic commented May 11, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyb-elastic commented May 15, 2018

andyb-elastic commented May 15, 2018

andyb-elastic commented May 15, 2018

andyb-elastic commented May 15, 2018

andyb-elastic commented May 15, 2018

andyb-elastic commented May 16, 2018

andyb-elastic commented May 16, 2018