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

Sensu-Go-Ctl support for windows platform #59

Merged
merged 82 commits into from
Oct 25, 2019
Merged

Sensu-Go-Ctl support for windows platform #59

merged 82 commits into from
Oct 25, 2019

Conversation

derekgroh
Copy link
Contributor

@derekgroh derekgroh commented Aug 30, 2019

Pull Request Checklist

#50

General

  • Update Changelog following the conventions laid out here

  • Update README with any necessary configuration snippets

  • Cookstyle (rubocop) passes

  • Foodcritic passes

  • Rspec (unit tests) passes
    Fails to detect the Download Sensuctl powershell_script, but strings match.

  • Inspec (integration tests) passes

New Features

  • Added a Testing Artifact as either an automated test or a manual artifact on the PR.

  • Added documentation for it to the README.md

Purpose

Provide a method for windows entities to install and configure the Sensuctl.exe application.

Known Compatibility Issues

This may be held until packaging for Sensuctl.exe is presented in a more Windows friendly manner avoiding the additional steps to obtain the exe, which would require modification to the resource and unit tests.

@derekgroh derekgroh requested a review from a team as a code owner August 30, 2019 15:44
@webframp
Copy link
Contributor

@derekgroh overall I'm 👍 for this, just a small question. Thanks for adding the windows support here!

Copy link
Contributor

@webframp webframp left a comment

Choose a reason for hiding this comment

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

@majormoses This looks reasonable to me. Approving but will hold off merging until you can comment as well.

Copy link
Contributor

@majormoses majormoses left a comment

Choose a reason for hiding this comment

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

I think we are almost there. Great work!

libraries/helpers_sensuctl.rb Outdated Show resolved Hide resolved
resources/ctl.rb Outdated Show resolved Hide resolved
resources/ctl.rb Outdated Show resolved Hide resolved
resources/ctl.rb Outdated Show resolved Hide resolved
@derekgroh
Copy link
Contributor Author

So a limitation of two different testing tools is there is no method through appveyor to test ctl recipes as it requires a converged server to make a call against. (Unless there is a public instance Sensu runs that we could make the call against?)

Should this be addressed as part of this PR or addressed later as the tooling allows?

@webframp
Copy link
Contributor

@derekgroh this is getting pretty big at this point so I'd like to be able to merge for you soon. Can you explain the appveyor problem a little more? Would there be a problem just using the same single instance on appveyor to run as the server + ctl for tests?

@derekgroh
Copy link
Contributor Author

@webframp Sensuctl suite tests a connection to a backend to confirm it is configured correctly, unless there is a way to mock it without a backend being available. Otherwise appveyor has no method to test this suite, because it can not launch a backend to test against. I can explain further in Slack if you want.

@webframp
Copy link
Contributor

@derekgroh Added #68 to remember to come back and address this testing
limitation.

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

Successfully merging this pull request may close these issues.

3 participants