-
Notifications
You must be signed in to change notification settings - Fork 260
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
Windows agent installation support #210
Conversation
Looking pretty awesome. I'll take a deeper review tomorrow and provide some feedback. |
@@ -21,6 +21,6 @@ end | |||
|
|||
group :integration do | |||
gem 'kitchen-vagrant' | |||
gem 'test-kitchen', '~> 1.3.1' | |||
gem 'test-kitchen' |
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.
Why the removal of any version constraint?
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 actually need ~> 1.4.0
for windows support, I'll change it.
@olivielpeau Looking good a bunch of comments. This supplies updates to chefSpec - what about some test-kitchen tests for this behavior? |
@miketheman Thanks a lot for your comments! I'll update the PR accordingly and add some test-kitchen tests. |
9c5582f
to
7a81e22
Compare
@miketheman I've updated the PR, it now has kitchen tests + solves the issues you pointed out. |
@@ -4,6 +4,9 @@ driver: | |||
customize: | |||
memory: 1024 | |||
|
|||
provisioner: | |||
data_path: test/shared # directory containing shared helpers for integration tests |
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 should not be needed - we already support shared directories via the test/integration/helpers
subdirectory, so this should also simplify the path handling.
If there's a common spec helper, then let's move all the common require
s and set path
into there.
@olivielpeau Awesome stuff! Ran through again, added some comments. It's looking better each pass. |
8ea13ce
to
d3e3e1b
Compare
Thanks @miketheman for the great review! I've addressed the issues you've found. I've left the file permissions on Linux unchanged for now: we have a ticket in our backlog to review them on all of our supported installation methods. |
d3e3e1b
to
280b5bf
Compare
Chef 11 and 12 include support for `windows_package` but it is still way behind in terms of reliability and design compared to the `windows_package` resource provided by the `windows` cookbook.
280b5bf
to
fac5177
Compare
The bats tests on dd-agent are replaced by serverspec tests
- spec_helper.rb is moved to test/integration/helpers/serverspec - move `require`s and `set :path` to the helper
- Remove all rights to regular user as they should not need any - Disable rights inheritance from parent directories
fac5177
to
6ced7e6
Compare
@miketheman I've fixed the issues you found, rebased the PR and re-run the kitchen tests so this is probably ready for a final review :) |
@olivielpeau Awesome! I think this is all ready to go, and will merge Monday. We can then start testing it internally, and hopefully find no bugs, then release to the world! |
Windows agent installation support
🎉 |
Much thanks on getting this merged and making it available in a shorter timeline! |
Reworked commits from PR #188 and #162 to support agent installation (and upgrade) and deploy checks configuration files on Windows.
Tested successfully on Chef 12. On Chef 11, during an installation run, the agent gets stuck in the 'Stopping' state when requested to stop or restart. I'm still tying to figure out if there's a good workaround.
Many thanks to @EasyAsABC123 and @rlaveycal for their initial PRs.
Closes #142, #188 and #164