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

Added support for Java VM options #37

Merged
merged 1 commit into from
Apr 12, 2013

Conversation

kbrezina
Copy link
Contributor

@@ -68,6 +68,8 @@
# all TCP connections).
# ['confdir'] - The puppetdb configuration directory; defaults to
# `/etc/puppetdb/conf.d`.
# ['java_args'] - Java VM options; e.g. { '-Xmx' => '256m', '-Xms' => '256m' };
# defaults to {}.

Choose a reason for hiding this comment

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

It's probably worth mentioning here what exactly that default means. If I'm reading your code correctly the default is to just leave the settings that were layed down by the package--which I believe is indeed the correct behavior, but reading the current docs doesn't make that entirely clear.

We might even want to include a link to the public puppetdb docs so folks can see what the defaults are.

@cprice404
Copy link

This looks good to me. Will hold off on merging it until after the inifile pull gets in. Thanks!

@@ -0,0 +1,15 @@
module Puppet::Parser::Functions
newfunction(:create_subsetting_resource_hash, :type => :rvalue) do |args|
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you namespace this with puppetdb_ ...? Just makes finding the source of the function easier for users :-).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I'll change the name

@cprice404
Copy link

@kbrezina thanks... I'm +1 on this as soon as we merge the inifile stuff. @kbarber we will need to be careful with the timing on merging this in, because doing so will require updaing the modulefile to depend on a newer version of inifile. If you are wanting to get a release out prior to that, then we should hold off on merging this until your release goes out.

@kbarber
Copy link
Contributor

kbarber commented Feb 25, 2013

@cprice-puppet how are we with this now? I wouldn't mind a release this week, but I can wait.

@cprice404
Copy link

@kbarber I think I still haven't merged his inifile pull req, so we're blocked on that.

@fhrbek
Copy link
Contributor

fhrbek commented Apr 12, 2013

I think this should be merged (the inifile changes have already been merged).

@kbarber
Copy link
Contributor

kbarber commented Apr 12, 2013

@fhrbek can you rebase the pull request, its out of date.

@kbrezina
Copy link
Contributor Author

Rebased.

kbarber added a commit that referenced this pull request Apr 12, 2013
@kbarber kbarber merged commit acf9e70 into puppetlabs:master Apr 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants