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 possibility to talk HTTPS in push client #38

Closed
wants to merge 4 commits into from

Conversation

colszowka
Copy link
Contributor

Currently, when passing a https URL to the push client, it bails out with an exception. This PR adds the possibility to also use HTTPS instead.

@coveralls
Copy link

coveralls commented Nov 21, 2016

Coverage Status

Coverage decreased (-0.4%) to 98.718% when pulling e7a22f6 on colszowka:allow-https into 5e16541 on prometheus:master.

@colszowka
Copy link
Contributor Author

colszowka commented Nov 21, 2016

So, there seems to be some issues with CI that are not related to my changes:

  • The build for Ruby 1.9 fails because the used term-ansicolor version only supports Ruby 2+
  • Rubocop complains about two issues that are not from my changes:
lib/prometheus/client/registry.rb:25:11: C: Use a guard clause instead of wrapping the code inside a conditional expression.
          if exist?(name.to_sym)
          ^^
spec/prometheus/client/counter_spec.rb:10:18: W: Use Integer instead of Fixnum.
    let(:type) { Fixnum }
                 ^^^^^^

One more issue Rubocop complains about actually is from my changes:

lib/prometheus/client/push.rb:50:9: C: Use a guard clause instead of wrapping the code inside a conditional expression.
        if SUPPORTED_SCHEMES.include? uri.scheme
        ^^

This is working the same way it used to in the original code though, and also since it uses the parsed URI I cannot make this a guard-clause easily. Also, personally I much prefer this conditional style to guard clauses, but that's not relevant :)

Also not sure why coverage dropped since the changes I made should actually all be covered.

@grobie
Copy link
Member

grobie commented Nov 21, 2016

Thanks you, this looks good to me in general. Both the 1.9 and rubocop issues are annoying. I think it's generally a good idea to explicitly support ruby 1.9 and test for it, but most library maintainers dropped 1.9 support already.

The great results by having clear formatting rules enforced by go fmt in golang projects are the reason I added rubocop to this project. Unfortunately, rubocop changes their recommended styles frequently and without any clear reasoning besides personal preferences (it seems to me). The guard clause rule doesn't make sense to me.

I can take a look at these issues, but I'm traveling right now. It might take a few days.

@coveralls
Copy link

coveralls commented Nov 21, 2016

Coverage Status

Coverage increased (+0.01%) to 99.103% when pulling a57f62f on colszowka:allow-https into 5e16541 on prometheus:master.

@colszowka
Copy link
Contributor Author

@grobie Thanks for the quick reply!

I have made adjustments to stabilize the CI build based on your feedback. Most notably I added a stricter version constraint for Rubocop, which should at the very least reduce any CI build drift based on changing rubocop rules. I think code analysis in Ruby is just much harder due to it being so dynamic and also much higher-level, the problem-space is just so much bigger, but getting different results on subsequent runs of the same code is definetely very annoying. This change should help (but introduces the inherent chore of actually bumping that number every now and then and working through the issues that come up from new rubocop versions).

I also disabled the Guard clause cop (we do the same for my simplecov gem ;) and fixed the one remaining rubocop warning from the existing codebase.

The changes are individual commits so we could revert them easily if needed.

@grobie grobie closed this in b055be7 Feb 14, 2017
@grobie
Copy link
Member

grobie commented Feb 14, 2017

I'm sorry for the silence on this issue, I've been traveling for the last couple of months. I rebased and merged your changes into master. Thank you for your contribution!

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.

None yet

3 participants