[TEST] packaging tests: add windows boxes #27682
Conversation
Vagrant boxes now include options for windows server 2012r2 and server 2016. Since licensing prevents us from providing windows images, they're only configured in vagrant if passed via environment variables. The windows boxes aren't used in gradle's packaging tests yet. For elastic#26741
andyb-elastic
added
:Delivery/Build
There was a problem hiding this comment.
Thanks for the small pull request @andyb-elastic, I've left few comments, mostly around documentation. I tested locally and I was able to start the windows images and get a shell. I don't really like the Vagrantfile.alternate.rb so I'd prefer to stick to the Vagrantfile.
Vagrantfile
Outdated
| powershell_install_deps config | ||
| end | ||
|
|
||
| def powershell_long_path_module |
There was a problem hiding this comment.
Can you add some documentation about what it does and what is robocopy?
There was a problem hiding this comment.
Sure - is the part above sufficient or are you looking for more detail https://github.com/elastic/elasticsearch/pull/27682/files#diff-23b6f443c01ea2efcb4f36eedfea9089R410
Windows' system APIs limit paths to 260 characters. In server 2016 we can raise this limit,
(see LongPathsEnabled) but not in server 2012r2. This adds a powershell module that wraps
robocopy, which can handle paths longer than the system limit. The wrapper converts
robocopy's unusual exit code and error handling to the behavior we'd normally expecthttps://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx https://ss64.com/nt/robocopy-exit.html
Vagrantfile
Outdated
| SHELL | ||
| end | ||
|
|
||
| def powershell_install_deps(config) |
| touch /etc/is_vagrant_vm | ||
| SHELL | ||
|
|
||
| windows_2012r2_box = ENV['VAGRANT_WINDOWS_2012R2_BOX'] |
There was a problem hiding this comment.
Can you update TESTING.asciidoc just to mention that Elastic uses two private Windows images identified by windows-21021r2 and windows-2016. The other information (real image names, env vars etc) should be documented in a different document.
There was a problem hiding this comment.
Sure thing - this was upcoming in the next PR but I'll add the parts relevant to the changes in this one
There was a problem hiding this comment.
other information (real image names, env vars etc) should be documented in a different document
I think it makes sense to document the env vars in TESTING.md so it's obvious how users can plug in their own windows images if need be. The real image names will definitely go elsewhere
Also removes alternate Vagrantfile For elastic#26741
|
@rjernst would like to hear your thoughts, mind taking a look when you have a minute? This is the first in a series of three PRs |
| config.vm.synced_folder vagrantfile_dir, '/elasticsearch' | ||
|
|
||
| # Expose project directory. Note that VAGRANT_CWD may not be the same as Dir.pwd | ||
| PROJECT_DIR = ENV['VAGRANT_PROJECT_DIR'] || Dir.pwd |
There was a problem hiding this comment.
What is the "project directory" compared to the vagrantfile_dir above? I don't think we need to (or should) expose anything but the dir containing the tests, especially since the fallback is the working dir, for which nothing in tests can rely on.
There was a problem hiding this comment.
This project directory gets set by gradle as the directory of the gradle project that applies the plugin and runs vagrant shell commands, so in this case qa/vagrant. This is where the bats tests run because everything they need is self-contained in the project. The new packaging tests need the entire gradle project. Once the bats tests have all been ported I think we can get rid of the project directory set here.
There was a problem hiding this comment.
But what good is pwd here as a fallback? Seems it would just be horribly broken assuming the working dir contains the tests, and we should instead fail if the env is not set.
There was a problem hiding this comment.
And also if that is the case this seems unrelated to the windows work? ie adding validation/error on it missing could be done in a separate PR.
There was a problem hiding this comment.
Right, it's unrelated to the windows stuff, it's just relocated from below. I'm not sure what the rationale is for falling back to pwd - I'd guess it's so that if you're making vagrant commands outside of gradle, you can just run them from the project directory and it'll mount it without you having to set the env var.
It would definitely be good to validate it, but it's going to be removed once all the bats tests are ported so I think it's okay to leave as-is. Happy to add some validation though if we think it's important in the meantime
There was a problem hiding this comment.
it's just relocated from below
Aha! This is why minimal changes are important. The PR made it look new, thus my comments.
| # http://foo-o-rama.com/vagrant--stdin-is-not-a-tty--fix.html | ||
| config.vm.provision "fix-no-tty", type: "shell" do |s| | ||
| config.vm.provision 'fix-no-tty', type: 'shell' do |s| |
There was a problem hiding this comment.
I see a lot of little changes like these double quotes to single quotes, or moving methods around below. Please keep these PRs to minimal changes. If you feel some cleanups should be made, open them in a separate PR. Superfluous changes make the PR harder to read.
|
Sure thing, the refactoring just helped me understand how the provisioning is organized. I'll close this, open a new PR for the refactoring/cleanup, then another PR for the new windows stuff |
Vagrant boxes now include options for windows server 2012r2 and server
2016. Since licensing prevents us from providing windows images, they're
only configured in vagrant if passed via environment variables. The
windows boxes aren't used in gradle's packaging tests yet.
For #26741
Appeared originally in #27623
The Vagrantfile.alternate.rb file is another version that I thought was more readable at first, if you're trying to figure out "what are the dependencies of all these boxes" - but it's less readable if you're trying to trace what goes into a specific box. I included it in case reviewers like that format better, otherwise I'll remove it before merging