[WIP] Pronto Integration (v2)

[NickLaMuro]

This is a rebase of #406 so please view that PR for further details.

Currently, it depends on #498 to be merged first, as they are both changing a lot of the same gems.

Rebased a few commits together to remove some commit noise, but otherwise mostly everything is the same as it was.

[NickLaMuro]

So we are probably going to need to change these two lines in the future to have the run in a different form. I manually fixed the files to get them into the "Pronto format", but that is not a good long term solution.

A few things worth noting here though:

• The spec/workers/commit_monitor_handlers/commit_range/rubocop_checker/data/*/ directories are not git dirs, so they don't play well with Pronto's runners, since they assume git.
• I had two potential approaches two this:
  • Convert those directories to git, and use a FakeResults class (or something) that inherits from the CodeAnalysisMixin to share that code
  • Create a set of fake libs for pronto that stub the git portions of the code to get the results we need.

I am not sure what is better at this point, because while the git approach is probably less code overall, we would either have to do a git init && git add . && git commit -m "Initial" as part of the block, or commit the .git dir for each one of these directories, which is... fun. The other option adds a bunch of code we need to support, so not really a big fan of that either.

But decided to sideline fixing that for now to move this forward.

[Fryguy]

Convert those directories to git, and use a FakeResults class (or something) that inherits from the CodeAnalysisMixin to share that code

This sounds good to me, though I don't know why we need the FakeResults class?

commit the .git dir for each one of these directories

We do this in ManageIQ proper, and we didn't have an issue with it, I think

[NickLaMuro]

This sounds good to me, though I don't know why we need the FakeResults class?

Because CodeAnalysisMixin is a module, and we need to be able to set @branch and such to be able to use .pronto_result

miq_bot/app/workers/concerns/code_analysis_mixin.rb

Lines 32 to 39 in 6bd031c

	def pronto_result
	Pronto::GemNames.new.to_a.each { |gem_name| require "pronto/#{gem_name}" }
	p_result = nil
	# temporary solution for: download repo, obtain changes, get pronto result about changes
	Dir.mktmpdir do |dir|
	FileUtils.copy_entry(@branch.repo.path.to_s, dir)

It would just be something really simple like:

class FakeResults
  include CodeAnalysisMixin

  def initialize(branch)
    @branch = branch
  end
end

# usage
FakeResults.new(branch).proto_results

We do this in ManageIQ proper, and we didn't have an issue with it, I think

Where? I am curious.

Also, not saying it isn't feasible, but it is kinda ugly checking in a bunch of extra git objects just to view a diff. I think I would rather have the unless File.exists? be a bit smarter, and do the git repo initialization inside the block (if regenerating, and blow it away if it exists), but not check it in to this repo.

[NickLaMuro]

We do this in ManageIQ proper, and we didn't have an issue with it, I think

Where? I am curious

Okay, I think you are talking about these:

https://github.com/ManageIQ/manageiq/tree/master/spec/fixtures/git_repos

Which those are bare repos. Probably would still work, but would rather avoid checking in a bunch of nest git files, as show for on of those:

$ tree spec/fixtures/git_repos/no_master.git/
spec/fixtures/git_repos/no_master.git/
├── HEAD
├── config
├── objects
│   ├── 43
│   │   └── 0ff1dcee43d2e8a12c3ba36f5ff20e46136152
│   ├── 44
│   │   └── f84e202a8eb9d8d35fc183ae8637db3b003aa8
│   ├── 78
│   │   └── 3878cb3ed5964c75767141176c80f7d528abb5
│   ├── 95
│   │   └── 99fdeb034597d90d72d2f58396dee096885b79
│   ├── ae
│   │   └── fde3a01f6e10d72fd4899ce14c8b2654d3eb45
│   ├── e3
│   │   └── 16f7d7548ea8f874c872ff3be04f4e9849833b
│   ├── e6
│   │   └── 9de29bb2d1d6434b8b29ae775ad8c2e48c5391
│   └── fd
│       └── b4bc8c22f4b3ca8279c2e1eb92670893473d05
├── packed-refs
└── refs
    └── tags
        ├── tag1
        └── tag2

11 directories, 13 files

Just for a few specs, especially when the git part is very inconsequential. It also seems like it would be hard to modify as end users as well.

[Fryguy]

Yeah those are it. FWIW, I think having those in core is also weird, so maybe whatever you choose we can do something similar in core as well.

[NickLaMuro]

While working on a CLI prototype, I ran into this above:

#500 (comment)

Marking this PR as [WIP] while I determine how to fix the git fetch with Rugged in the bot on prod. Would be nice to have that in a more central place going forward as well, as the current TODO item in app/models/repo.rb suggests

[NickLaMuro]

So working on getting this across the finish line:

The last commit simplifies the git fetch ... that was being done, and hopefully avoids a bit of extra work.

However, it seems like I might need to tweak some things, because currently it isn't reporting any problems in a PR where I introduced them:

• ✅ Good example: Testing Proto formatting manageiq-ui-classic#7299 (comment)
• ❌ Bad linting (via this PR's code): Testing Proto formatting GrumpyTech/manageiq-ui-classic#2

(Edit: The "Good example" was actually a "happy accident" (mistake) that I accidentally opened, but turned out to be helpful 😆 🌲 🌲 )

(Edit 2: Forgot the bot will delete old versions of the comment, so I can't "permalink" to it while I run some tests. The screenshot will be a historical representation of what it was before I made some fixes)

Might be related to this comment by Jason:

https://github.com/ManageIQ/miq_bot/pull/406/files#r170294743

But will report back when I find out more.

[NickLaMuro]

Been struggling with this for a few days now, but our configs for RuboCop aren't being loaded, and the most recent two commits are attempting to address that, but still aren't working.

Will look more tomorrow and such...

[miq-bot]

Checked commits NickLaMuro/miq_bot@aad33e6~...5331ffc with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
9 files checked, 7 offenses detected

Gemfile

• ❗ - Line 44, Col 1 - Bundler/OrderedGems - Gems should be sorted in an alphabetical order within their section of the Gemfile. Gem manageiq-style should appear before more_core_extensions.

app/workers/concerns/code_analysis_mixin.rb

• ⚠️ - Line 72, Col 39 - Lint/AssignmentInCondition - Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.
• ❗ - Line 133, Col 7 - Layout/AlignHash - Align the elements of a hash literal if they span more than one line.
• ❗ - Line 70, Col 32 - Security/YAMLLoad - Prefer using YAML.safe_load over YAML.load.
• ❗ - Line 75, Col 42 - Layout/ExtraSpacing - Unnecessary spacing detected.
• ❗ - Line 75, Col 44 - Layout/SpaceAroundOperators - Operator << should be surrounded by a single space.
• ❗ - Line 89, Col 5 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

[NickLaMuro]

So, had a frustrating realization... and unfortunately, it isn't an issue with configs being loaded incorrectly...

Turns out that Pronto doesn't do what we do, which is looks up a "blob" lookup via Rugged to checkout a version of the file that is to be analyzed:

https://github.com/ManageIQ/miq_bot/blob/master/lib/linter/base.rb#L56

And instead, basically assumes that you have checked out the content of the entire branch to disk to then run pronto against it:

https://github.com/prontolabs/pronto#local-changes
https://github.com/prontolabs/pronto-rubocop/blob/master/lib/pronto/rubocop/patch_cop.rb#L22-L27

Which is just not how miq_bot is designed to function...

Really kinda sucks, because I honestly think this is just going to be more trouble than it is worth to convert to using pronto now since they just have a completely different paradigm for how they interact with git. We could work around it with patches and such, but that seems like a lot of work and something tells me that this is not going to be a pattern that they want to introduce to their main code base.

Do we start from scratch? Do just continue with our workflow? Do we extract what we have into our own gem? Not really sure at the moment what makes the most sense.

[miq-bot]

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

[NickLaMuro]

Going to close as I don't see this happening. Can reopen if it does.