Skip to content
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

Merged
merged 17 commits into from
Oct 22, 2015
Merged

Conversation

olivielpeau
Copy link
Member

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

@olivielpeau olivielpeau added this to the Windows! milestone Jun 10, 2015
@miketheman
Copy link
Contributor

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'
Copy link
Contributor

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?

Copy link
Member Author

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.

@miketheman
Copy link
Contributor

@olivielpeau Looking good a bunch of comments.

This supplies updates to chefSpec - what about some test-kitchen tests for this behavior?

@olivielpeau
Copy link
Member Author

@miketheman Thanks a lot for your comments! I'll update the PR accordingly and add some test-kitchen tests.

@olivielpeau
Copy link
Member Author

@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
Copy link
Contributor

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 requires and set path into there.

@miketheman
Copy link
Contributor

@olivielpeau Awesome stuff! Ran through again, added some comments. It's looking better each pass.

@olivielpeau olivielpeau force-pushed the olivielpeau/windows-support branch 2 times, most recently from 8ea13ce to d3e3e1b Compare July 7, 2015 23:35
@olivielpeau
Copy link
Member Author

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.

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
@olivielpeau
Copy link
Member Author

@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 :)

@miketheman
Copy link
Contributor

@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!

@miketheman miketheman self-assigned this Oct 17, 2015
miketheman added a commit that referenced this pull request Oct 22, 2015
@miketheman miketheman merged commit d653883 into master Oct 22, 2015
@remh
Copy link
Contributor

remh commented Oct 22, 2015

🎉

@jboeshart
Copy link

Much thanks on getting this merged and making it available in a shorter timeline!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants