Skip to content

Initial implementation #1

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 9, 2016
Merged

Initial implementation #1

merged 1 commit into from
Mar 9, 2016

Conversation

pbrisbin
Copy link
Contributor

@pbrisbin pbrisbin commented Mar 8, 2016

/cc @mrb @codeclimate/review

The current engine is deployed from here, which includes some
bug-fixes to the underlying tool.

This repo copied over its wrapper script with the following changes:

  • General refactor

  • Use the tool as-is

  • Pass (essentially) include_paths/**/*.rb as cookbook paths

  • Don't use the tool's exclude_paths feature

  • Support config.tags

  • Support config.cookbook_paths

    NOTE: If present, our own computed include_paths are ignored

  • Hide issues on non-existent files

Comparing on an example repo:

Existing engine: https://codeclimate.com/github/mrb/mysql/issues

This version (required excluding test and spec):

== libraries/provider_mysql_service_base.rb (1 issue) ==
78: Avoid repetition of resource declarations [foodcritic]

== libraries/provider_mysql_service_smf.rb (1 issue) ==
6: Prefer conditional attributes [foodcritic]

== libraries/provider_mysql_service_systemd.rb (1 issue) ==
6: Prefer conditional attributes [foodcritic]

== libraries/provider_mysql_service_upstart.rb (1 issue) ==
6: Prefer conditional attributes [foodcritic]

(The same.)

@pbrisbin
Copy link
Contributor Author

pbrisbin commented Mar 8, 2016

I should probably write some tests too.

@wfleming
Copy link

wfleming commented Mar 8, 2016

i-should-write-s0z31y

wfleming
wfleming reviewed Mar 8, 2016
View reviewed changes
Dockerfile Outdated
Copy link

Choose a reason for hiding this comment

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

Isn't there a -slim version of this we can use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll look and hope it compiles -- been running into some nokogiri issues.

Copy link

Choose a reason for hiding this comment

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

Oh, yeah, I think you need the ruby-dev package (& maybe some build tools as well), which -slim images don't come with. Bummer. If we don't need 2.3, maybe stick with our alpine image for now? The full Ruby image is pretty large, if I recall. (I should really make time to update our alpine image for newer Rubies).

@pbrisbin
Copy link
Contributor Author

pbrisbin commented Mar 8, 2016

OK, this is ready for final review.

@mrb once merged, how would you like me to proceed on this? I propose:

  • Get your approval (i.e. please test it)
  • Open source this repo
  • Add to submissions
  • Ship it as the new version of this engine
  • Update docs accordingly (see this repo's README)

@jpignata
Copy link
Contributor

jpignata commented Mar 9, 2016

I tested this engine across a a handful of repos from https://github.com/chef-cookbooks and it looks 👍 to me.

@pbrisbin
Initial implementation
4fffb29 
This is an extracted combination of mrb/foodcritic (the
code_climate_support branch) and chef/codeclimate-foodcritic and is
meant to obviate both of those in a codeclimate-community-maintained
version.

It has the following notable differences from those projects:

- Invokes foodcritic as-is (does not rely on bugfixes in the mrb fork)
- Expands and filters include_paths into .rb files as necessary
- Supports config.tags
- Supports config.cookbook_paths (means ignoring include_paths)
- Hides issues on non-existent files (CC does not support this)
@pbrisbin
Copy link
Contributor Author

pbrisbin commented Mar 9, 2016

@jpignata I've rebased with (hopefully) a good commit message. If you agree, please feel free to merge, open source this repo, and begin the process of getting it in front of the customer.

I have to run, but hope to be back online ~10:15.

jpignata added a commit that referenced this pull request Mar 9, 2016
@jpignata
Merge pull request #1 from codeclimate-community/pb-initial
214aeec 
Initial implementation
@jpignata jpignata merged commit 214aeec into master Mar 9, 2016
@jpignata jpignata deleted the pb-initial branch March 9, 2016 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pbrisbin @wfleming @jpignata