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

Fix rubocop violation for fluentd/fluent-plugin-loki #1646

Merged
merged 6 commits into from
Feb 6, 2020

Conversation

takanabe
Copy link
Contributor

@takanabe takanabe commented Feb 6, 2020

What this PR does / why we need it:

For the maintainability of fluentd output plugin

Which issue(s) this PR fixes:
Fixes #1645

Special notes for your reviewer:

I want to refactor fluentd plugin code through #1645 but it may take time and many contexts. For the first step, I want to fix rubocop violation during rake spec

```
lib/fluent/plugin/out_loki.rb:76:21: C: Style/ClassCheck: Prefer
Object#is_a? over Object#kind_of?.
        unless @uri.kind_of?(URI::HTTP) or @uri.kind_of?(URI::HTTPS)
                    ^^^^^^^^
```
```
lib/fluent/plugin/out_loki.rb:78:9: C: Layout/EmptyLineAfterGuardClause:
Add empty line after guard clause.
        end
```
```
lib/fluent/plugin/out_loki.rb:76:38: C: Style/AndOr: Use || instead of
or.
        unless @uri.is_a?(URI::HTTP) or @uri.is_a?(URI::HTTPS)
                                     ^^
```
```
lib/fluent/plugin/out_loki.rb:77:38: C: Style/StringLiterals: Prefer
single-quoted strings when you don't need string interpolation or
special symbols.
          raise Fluent::ConfigError, "url parameter must be valid HTTP"
                                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```
Both rubocop and rspec fail with the current codes. Except for this
rubocop violation, they were fixed. this cyclomatic complexity violation
occurrs in configure method. However, it's still simple and the
implementation could be changed during spec fixation. So, this violation
should be ignored at this moment.

```
lib/fluent/plugin/out_loki.rb:72:7: C: Metrics/CyclomaticComplexity:
Cyclomatic complexity for configure is too high. [7/6]
      def configure(conf) ...
      ^^^^^^^^^^^^^^^^^^^
```
@codecov-io
Copy link

Codecov Report

Merging #1646 into master will increase coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1646      +/-   ##
==========================================
+ Coverage   61.84%   61.89%   +0.04%     
==========================================
  Files         109      109              
  Lines        8304     8304              
==========================================
+ Hits         5136     5140       +4     
+ Misses       2774     2772       -2     
+ Partials      394      392       -2
Impacted Files Coverage Δ
pkg/ingester/transfer.go 66.42% <0%> (+1.42%) ⬆️
pkg/promtail/targets/tailer.go 78.16% <0%> (+2.29%) ⬆️

@takanabe takanabe changed the title Fix rubocop violation Fix rubocop violation for fluentd/fluent-plugin-loki Feb 6, 2020
@cyriltovena
Copy link
Contributor

I did a similar cleanup a month ago, the reason why it doesn't stay clean is because we don't have a CI for it and we should but time is hard to find.

I really appreciate your time ! Thanks !

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

@cyriltovena cyriltovena merged commit 8f8cb80 into grafana:master Feb 6, 2020
@takanabe takanabe deleted the fix_rubocop_violation branch February 6, 2020 23:59
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.

Hard to debug and refactor fluent-plugin-grafana-loki
3 participants