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.

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

Replace Accumulator Resources With Definitions #228

Closed
wants to merge 12 commits into from
Closed

Replace Accumulator Resources With Definitions #228

wants to merge 12 commits into from

Conversation

zuazo
Copy link
Contributor

@zuazo zuazo commented Jun 18, 2015

Hi,

This is my attempt to try to complete the work started by @cwjohnston in the accumulator-definitions branch. The following PR replaces all the accumulator resources by accumulator definitions as it has been discussed in #197.

Some clarifications:

  • Replaced the file resources by templates to avoid using run_state unnecessarily.
  • Added more test-kitchen & Serverspec integration tests.
  • Moved the whisper installation from the carbon_cache definition to the carbon_conf_accumulator definition: All the carbon daemons depend on it.
  • I think the ChefGraphite.ini_file method can be simplified a lot and perhaps even move some of the logic to the carbon.conf.erb template. But I left it as it is to avoid adding too many changes.
  • The graphite_example::single_node recipe seems to be broken in both master and in this PR (due to issue "ImportError: No module named fields" when install #227).
  • The create_graphite_carbon_conf_accumulator ChefSpec matcher is a bit tricky. It will not work for all cases. But I have not found a simple way to create ChefSpec matchers for definitions.

@zuazo zuazo mentioned this pull request Jun 18, 2015
Closed
@cwjohnston
Copy link
Contributor

Hi Xabier,

Thanks for taking this on, I am very keen to review the changes. Could you
please re-create this PR against the "develop" branch?

On Thursday, June 18, 2015, Xabier de Zuazo [email protected]
wrote:

Hi,

This is my attempt to try to complete the work started by @cwjohnston
https://github.com/cwjohnston in the accumulator-definitions branch
https://github.com/hw-cookbooks/graphite/compare/accumulator-definitions?diff=unified&name=accumulator-definitions.
The following PR replaces all the accumulator resources by accumulator
definitions as it has been discussed in #197
#197.

Some clarifications:

  • Replaced the file resources by templates to avoid using
    run_state unnecessarily.
  • Added more test-kitchen & Serverspec integration tests.
  • Moved the whisper installation from the carbon_cache definition to
    the carbon_conf_accumulator definition: All the carbon daemons
    depend on it.
  • I think the ChefGraphite.ini_file method can be simplified a lot and
    perhaps even move some of the logic to the carbon.conf.erb template.
    But I left it as it is to avoid adding too many changes.
  • The graphite_example::single_node recipe seems to be broken in both
    master and in this PR (due to issue "ImportError: No module named fields" when install #227
    "ImportError: No module named fields" when install #227).
  • The create_graphite_carbon_conf_accumulator ChefSpec matcher is a
    bit tricky. It will not work for all cases. But I have not found a simple
    way to create ChefSpec matchers for definitions.

You can view, comment on, or merge this pull request online at:

#228
Commit Summary

  • bump version to 1.0.3 for development
  • remove invocations of use_inline_resources
  • replace accumulator resources with definitions
  • Add more complete integration tests: aggregator, cache, relay
  • Merge remote-tracking branch
    'remotes/origin/accumulator-definitions' into accumulator-definitions-2
  • Add integration tests to check carbon processes
  • Implement carbon and storage_schemas definitions using templates
  • Gemfile: Fix unit tests in Ruby 1.9.3
  • README: Update accumulator pattern documentation
  • graphite_carbon_conf_accumulator: Do not install whisper with
    :delete action

File Changes

Patch Links:

—
Reply to this email directly or view it on GitHub
#228.

@zuazo zuazo force-pushed the accumulator-definitions-2 branch from fab355b to c1428e0 Compare June 19, 2015 03:12
@zuazo zuazo mentioned this pull request Jun 19, 2015
Closed
@zuazo
Copy link
Contributor Author

zuazo commented Jun 19, 2015

Hi @cwjohnston,

Thanks for your fast response. I rebased this onto develop and squashed some commits: #230

@zuazo zuazo closed this Jun 19, 2015
@webframp webframp mentioned this pull request Oct 29, 2016
Closed
@lock
Copy link

lock bot commented Apr 25, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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
@zuazo @cwjohnston @webframp