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

Add Pulp profiling option #178

Merged
merged 1 commit into from Mar 17, 2017

Conversation

Projects
None yet
4 participants
@ehelms
Copy link
Member

ehelms commented Dec 7, 2016

No description provided.

@ehelms

This comment has been minimized.

Copy link
Member Author

ehelms commented Dec 7, 2016

Not to be merged until pulp/pulp#2882 is merged and released in a Pulp release

@ehelms ehelms changed the title Add Pulp profiling option [DNM[ Add Pulp profiling option Dec 7, 2016

@ehelms ehelms changed the title [DNM[ Add Pulp profiling option [DO NOT MERGE] Add Pulp profiling option Dec 7, 2016

@ehelms ehelms changed the title [DO NOT MERGE] Add Pulp profiling option Add Pulp profiling option Feb 23, 2017

@parthaa

This comment has been minimized.

Copy link
Contributor

parthaa commented Feb 27, 2017

@ehelms do you want to close http://projects.theforeman.org/issues/18617 or use this PR for that?

@ehelms ehelms force-pushed the ehelms:add-profiling branch 2 times, most recently from 658e3fa to 4ba2f8e Feb 28, 2017

@ehelms

This comment has been minimized.

Copy link
Member Author

ehelms commented Feb 28, 2017

This is passing tests now. @parthaa want to review it?

@stbenjam
Copy link
Member

stbenjam left a comment

From the puppet side of things, just one small comment

@@ -377,6 +382,8 @@
$puppet_wsgi_processes = $pulp::params::puppet_wsgi_processes,
$migrate_db_timeout = $pulp::params::migrate_db_timeout,
$show_conf_diff = $pulp::params::show_conf_diff,
$enable_profiling = $pulp::params::enable_profiling,
$profiling_directory = $pulp::params::profiling_directory,

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 2, 2017

Member

Would be good to have some validations on these, validate_bool and validate_absolute_path respectively.

# $enable_profiling:: Turns on cProfiling of tasks in Pulp
# type:boolean
#
# $profiling_directory:: Directory to store task profling data in

This comment has been minimized.

Copy link
@ekohl

ekohl Mar 3, 2017

Member

s/profling/profiling/

@ehelms ehelms force-pushed the ehelms:add-profiling branch from 4ba2f8e to 0aaa71f Mar 9, 2017

@ehelms

This comment has been minimized.

Copy link
Member Author

ehelms commented Mar 9, 2017

@ekohl @stbenjam updated, please review

# writeable and readable by Pulp.

[profiling]
<% unless [nil, :undefined, :undef, ''].include?(scope.lookupvar('pulp::enable_profiling')) -%>

This comment has been minimized.

Copy link
@ekohl

ekohl Mar 9, 2017

Member

We validate it's a boolean so this isn't needed.

<% unless [nil, :undefined, :undef, ''].include?(scope.lookupvar('pulp::enable_profiling')) -%>
enable: <%= scope['pulp::enable_profiling'] %>
<% end -%>
<% unless [nil, :undefined, :undef, ''].include?(scope.lookupvar('pulp::profiling_directory')) -%>

This comment has been minimized.

Copy link
@ekohl

ekohl Mar 9, 2017

Member

We validate it's a valid path so this isn't needed.

@ehelms ehelms force-pushed the ehelms:add-profiling branch from 0aaa71f to d66bd5f Mar 17, 2017

@ehelms ehelms force-pushed the ehelms:add-profiling branch from d66bd5f to 1fdec14 Mar 17, 2017

@ehelms

This comment has been minimized.

Copy link
Member Author

ehelms commented Mar 17, 2017

Updated and rebased

@ekohl

ekohl approved these changes Mar 17, 2017

@ehelms ehelms merged commit b4c9001 into theforeman:master Mar 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.