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

If the graph directory is provided, make sure it's created first #216

Closed
wants to merge 3 commits into from

Conversation

jontg
Copy link

@jontg jontg commented Oct 24, 2014

No description provided.

@tduffield
Copy link
Contributor

Thanks for the PR. There are a couple things I'd like to work with you on before we accept the PR.

  1. I just want to confirm that the graph directory is relevant only to upstart and not any of the other service management types.
  2. There are multiple difference commits going on here. Can you remove the commits related to the upstream merge and cacheing the key. That should be addressed in a separate commit.
  3. Can you put the check for the node attribute as a chef resource guard instead of a conditional check?
  4. Please add a chefspec test case to test your behavior.

If you do those three things we should be able to move forward with the merge.
Thanks!

@jontg
Copy link
Author

jontg commented Oct 28, 2014

Re-opening in #220 to address task 2, above.

@jontg jontg closed this Oct 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants