osuosl-cookbooks / pgd

Apache License 2.0
1 stars 0 forks source link

Made PGD Cookbook compatible with PGD Dev Branch #12

Closed pop closed 9 years ago

pop commented 9 years ago

Refs #9

ramereth commented 9 years ago

This needs to be rebased on master as its not mergable right now.

mathuin commented 9 years ago

The point of this pull request is for this branch of the cookbook to be compatible with the develop branch of PGD. It's okay that this pull request is not mergable at the present time -- it is intended to be merged when PGD version 1.1 is released, which is when the PGD develop branch will be merged back into the PGD master branch.

ramereth commented 9 years ago

We need to account for changes that happen for master no matter what happens on the 1.1 side of things IMO. But I'll let @jordane chime in if he feels differently.

ramereth commented 9 years ago

A few things I notice off the bat:

  1. Why are we starting apache in default.rb? The apache cookbook should be doing that...
  2. IMO we should be creating a pgd system user and set a default location to install the app no matter if we're on vagrant/openstack/whatever. Something like /opt/pgd might work or something in /var/lib/pgd.
  3. You might run berks update and see what happens. It will likely break because the newer mysql cookbook has different dependencies. I already mentioned this in #14 but we may want to put the fix here instead. For now we should match the version we have in production since that's what we're using but we'll eventually want to use the newer one. Right now the test kitchen run breaks on an updated berk install.
  4. Running foodcritic . is also a good thing to do to catch chef specific linting problems.
  5. Its good to add .rubocop.yml and run it to keep your code up to style standards. Here's an example file you can use:
AllCops:
  Include:
    - '**/Berksfile'
    - '**/Cheffile'
  Exclude:
    - 'metadata.rb'
Lint/AmbiguousRegexpLiteral:
  Exclude:
    - 'test/integration/**/*.rb'

Looks mostly good though! Let's try and get #14 merged ASAP and then rebase this on that

jordane commented 9 years ago

@ramereth I don't think we want to exclude test/integration/**/*.rb from rubocop..

ramereth commented 9 years ago

@jordane look at it more closely. That's ONLY excluding the regex tests that we want to keep anyways because of server spec's way of doing things.

jordane commented 9 years ago

@ramereth Ah, derp. I can't read.

pop commented 9 years ago

@ramereth I have created issues for the first two points you made above and have created an issue for including a .rubocop.yml. The rest of your comments I will complete and push asap. The issues should be resolved before this pull request is completed.