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

[WIP] Unclutter commit monitor #405

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Fryguy
Copy link
Member

@Fryguy Fryguy commented Feb 9, 2018

This PR unclutters the core of the CommitMonitor by removing the whole categorization notion of separate workers. All workers will receive the branch_id and the new_commits on that branch, and it is up to the worker how they want to deal with that.

@bdunne Please review.

PR is WIP because I haven't tested enough yet.

@Fryguy
Copy link
Member Author

Fryguy commented Feb 9, 2018

First 2 commits are flattening the commit_range directory. The last commit is the big change to the CommitMonitor

@Fryguy
Copy link
Member Author

Fryguy commented Feb 16, 2018

Did a quick first pass on testing and something is wrong. It seems to execute all of the handlers even if there haven't been any changes to the branch. I must have removed a check somewhere in all those deletions. No issues though when the actual workers run.

@miq-bot
Copy link
Member

miq-bot commented Mar 2, 2018

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Mar 9, 2018

Checked commits Fryguy/miq_bot@b30558e~...879a668 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
32 files checked, 330 offenses detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - missing config files

app/workers/commit_monitor.rb

app/workers/commit_monitor_handlers/bugzilla_commenter.rb

app/workers/commit_monitor_handlers/bugzilla_pr_checker.rb

app/workers/commit_monitor_handlers/github_pr_commenter/diff_filename_checker.rb

  • ❗ - Line 42, Col 10 - Rails/TimeZone - Do not use Time.parse without zone. Use one of Time.zone.parse, Time.current, Time.parse.in_time_zone, Time.parse.utc, Time.parse.getlocal, Time.parse.iso8601, Time.parse.jisx0301, Time.parse.rfc3339, Time.parse.to_i, Time.parse.to_f instead.

app/workers/commit_monitor_handlers/path_based_labeler.rb

app/workers/commit_monitor_handlers/rubocop_checker.rb

  • ❗ - Line 24, Col 5 - Rails/Present - Use if @results["files"].present? instead of unless @results["files"].blank?.

app/workers/commit_monitor_handlers/rubocop_checker/message_builder.rb

app/workers/commit_monitor_handlers/rubocop_checker/rubocop_results_filter.rb

spec/workers/commit_monitor_handlers/github_pr_commenter/diff_content_checker_spec.rb

spec/workers/commit_monitor_handlers/rubocop_checker/data/with_lines_not_in_the_diff/example.rb

spec/workers/commit_monitor_handlers/rubocop_checker/data/with_results_generating_multiple_comments/lots_of_issues.rb

@miq-bot
Copy link
Member

miq-bot commented Mar 9, 2018

...continued

spec/workers/commit_monitor_handlers/rubocop_checker/data/with_results_with_offenses/coding_convention.rb

  • ❗ - Line 3, Col 5 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
  • ❗ - Line 4, Col 5 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.

spec/workers/commit_monitor_handlers/rubocop_checker/data/with_results_with_offenses/ruby_syntax_error.rb

  • 💣 💥 🔥 🚒 - Line 3, Col 1 - Lint/Syntax - unexpected token kEND
    (Using Ruby 2.3 parser; configure using TargetRubyVersion parameter, under AllCops)

spec/workers/commit_monitor_handlers/rubocop_checker/data/with_results_with_offenses/ruby_warning.rb

spec/workers/commit_monitor_handlers/rubocop_checker/data/with_void_warnings_in_spec_files/non_spec_file_with_void_warning.rb

spec/workers/commit_monitor_handlers/rubocop_checker/data/with_void_warnings_in_spec_files/spec/non_spec_file_in_spec_dir_with_void_warning.rb

spec/workers/commit_monitor_handlers/rubocop_checker/message_builder_spec.rb

@miq-bot
Copy link
Member

miq-bot commented Mar 5, 2019

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Mar 6, 2023

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@Fryguy
Copy link
Member Author

Fryguy commented Mar 10, 2023

Reopening cause I still think this is the right approacj

@Fryguy Fryguy reopened this Mar 10, 2023
@Fryguy Fryguy added pinned and removed stale labels Mar 10, 2023
@miq-bot
Copy link
Member

miq-bot commented Mar 16, 2023

This pull request is not mergeable. Please rebase and repush.

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

Successfully merging this pull request may close these issues.

None yet

4 participants