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

add create and delete actions for windows_service #6595

Conversation

jasonwbarnett
Copy link
Contributor

@jasonwbarnett jasonwbarnett commented Nov 21, 2017

Description

adds :create, :delete, and :configure actions to the windows_service resource.

Issues Resolved

Check List

@jasonwbarnett jasonwbarnett requested a review from a team November 21, 2017 22:07
@jasonwbarnett jasonwbarnett force-pushed the feature/add-action_create-to-windows_service branch from c045e58 to 97415ff Compare November 21, 2017 22:10
@btm
Copy link
Contributor

btm commented Nov 21, 2017

Thanks @jasonwbarnett, looks like you've put some work and thought into this.

Appveyor is still broken here, but you'll see on Travis that win32/windows/constants isn't part of the standard library. The win32 gems are only dependencies on Windows, so you'll need some conditionals and/or rspec filters around those requirements.

Please look at adding unit and functional test coverage for the new actions. There are some changes to existing functionality here so fixing up functional tests first could help ensure we don't have regressions.

And take a look at RFC082 regarding deprecating functionality.

@jasonwbarnett
Copy link
Contributor Author

@btm I wanted to submit it to get feedback sooner, rather than later. I'll be sure to update functional test and add new unit/functional tests.

There are a ton of constants in the win32-service gem. Does it make sense to copy those constants into the chef code base to eliminate the dependency across all operating systems? I'm looking for a little guidance here.

@jasonwbarnett jasonwbarnett force-pushed the feature/add-action_create-to-windows_service branch from 97415ff to 6c0e079 Compare November 21, 2017 23:42
@btm
Copy link
Contributor

btm commented Nov 22, 2017

If you're going to use most of the constants, I'd probably copy the file in and copy the license and copyright in it. I'll check internally if we're okay with that license-wise.

@btm
Copy link
Contributor

btm commented Nov 22, 2017

Yeah, we'd be okay with copying that constants file to Chef in regards to licensing.

@jasonwbarnett jasonwbarnett force-pushed the feature/add-action_create-to-windows_service branch 2 times, most recently from 5ac27a4 to b1b8894 Compare November 23, 2017 22:26
…esource.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the
    best of my knowledge, is covered under an appropriate open
    source license and I have the right under that license to
    submit that work with modifications, whether created in whole
    or in part by me, under the same open source license (unless
    I am permitted to submit under a different license), as
    Indicated in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including
    all personal information I submit with it, including my
    sign-off) is maintained indefinitely and may be redistributed
    consistent with this project or the open source license(s)
    involved.

Signed-off-by: Jason Barnett <jason.w.barnett@gmail.com>
Copy link
Contributor

@thommay thommay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tenor-72093569

@thommay thommay merged commit 91c1ab1 into chef:master Jan 22, 2018
@thommay
Copy link
Contributor

thommay commented Jan 22, 2018

Thanks for this great addition!

@lamont-granquist
Copy link
Contributor

So this needed a bunch of rspec work to fix travis + appveyor, things got very angry + red

@jasonwbarnett
Copy link
Contributor Author

I’ll try to fix this up at some point. I just haven’t had time to finish this work fully.

@jasonwbarnett
Copy link
Contributor Author

ok- starting to work on this now. I'll let you know when I have a solid suite of tests covering new functionality + existing tests working :)

@lock
Copy link

lock bot commented Apr 14, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked and limited conversation to collaborators Apr 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants