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

Implement TLS connections, default to TLS connection #2

Merged
merged 1 commit into from Jun 13, 2016

Conversation

Projects
None yet
2 participants
@GhostLyrics
Contributor

GhostLyrics commented Jun 6, 2016

Pull Request Checklist

Is this in reference to an existing issue? No

General

  • Update Changelog following the conventions laid out on Keep A Changelog
  • Update README with any necessary configuration snippets
  • Binstubs are created if needed
  • RuboCop passes
  • Existing tests pass

New Plugins

  • Tests
  • Add the plugin to the README
  • Does it have a complete header as outlined here

Purpose

Changes default plain text connection to TLS.

Known Compatablity Issues

(btw, there's a typo in the your header ;) )

It's 2016, we have Let's Encrypt. (and we're not deprecating plain text
connections. In fact we make an educated guess which ports are going to
be used.)

@GhostLyrics

This comment has been minimized.

Show comment
Hide comment
@GhostLyrics

GhostLyrics Jun 6, 2016

Contributor

Sorry for the amount of rebases and force pushes. Had issues with rubocop and then remembered about format string for integers.

Contributor

GhostLyrics commented Jun 6, 2016

Sorry for the amount of rebases and force pushes. Had issues with rubocop and then remembered about format string for integers.

@cwjohnston

This comment has been minimized.

Show comment
Hide comment
@cwjohnston

cwjohnston Jun 8, 2016

Contributor

Hi @GhostLyrics, thanks for the PR. I think that using TLS by default is reasonable from the perspective of preferring security, but I believe it means we will need to cut the next release with a major version change. Incrementing the major version in next release isn't a blocker, but it would be great to have some tests around the logic used in selecting TLS vs plain text connections.

My desire for tests aside, can you please rebase? I think updating changelog in #3 has introduced a conflict here.

Contributor

cwjohnston commented Jun 8, 2016

Hi @GhostLyrics, thanks for the PR. I think that using TLS by default is reasonable from the perspective of preferring security, but I believe it means we will need to cut the next release with a major version change. Incrementing the major version in next release isn't a blocker, but it would be great to have some tests around the logic used in selecting TLS vs plain text connections.

My desire for tests aside, can you please rebase? I think updating changelog in #3 has introduced a conflict here.

@GhostLyrics

This comment has been minimized.

Show comment
Hide comment
@GhostLyrics

GhostLyrics Jun 8, 2016

Contributor

Will add tests as mention on the sensu IRC.

Contributor

GhostLyrics commented Jun 8, 2016

Will add tests as mention on the sensu IRC.

@GhostLyrics

This comment has been minimized.

Show comment
Hide comment
@GhostLyrics

GhostLyrics Jun 9, 2016

Contributor

Rebased. After having a second look I'm not sure what to test though. The whole difference between TLS and plain text connections is one line which is switched by an if/elsif/else.

Contributor

GhostLyrics commented Jun 9, 2016

Rebased. After having a second look I'm not sure what to test though. The whole difference between TLS and plain text connections is one line which is switched by an if/elsif/else.

Implement TLS connections, default to TLS connection
It's 2016, we have Let's Encrypt. (and we're not deprecating plain text
connections. In fact we make an educated guess which ports are going to
be used.)
@GhostLyrics

This comment has been minimized.

Show comment
Hide comment
@GhostLyrics

GhostLyrics Jun 13, 2016

Contributor

Added tests, fixed a logic error while writing the tests. (although it feels as if I'm testing the detailed implementation :/ )

Contributor

GhostLyrics commented Jun 13, 2016

Added tests, fixed a logic error while writing the tests. (although it feels as if I'm testing the detailed implementation :/ )

@cwjohnston

This comment has been minimized.

Show comment
Hide comment
@cwjohnston

cwjohnston Jun 13, 2016

Contributor

@GhostLyrics thanks for writing these tests! 🎉

I agree that testing the specific implementation is less than desirable, but in this case I'm not sure what to suggest otherwise. I'm 👍 on this change.

Contributor

cwjohnston commented Jun 13, 2016

@GhostLyrics thanks for writing these tests! 🎉

I agree that testing the specific implementation is less than desirable, but in this case I'm not sure what to suggest otherwise. I'm 👍 on this change.

@cwjohnston cwjohnston merged commit 9fc5556 into sensu-plugins:master Jun 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment