-
Notifications
You must be signed in to change notification settings - Fork 23
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
Adding kb0287.json and kb0287.sh #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This task looks great to me. I have a few small formatting issues to keep consistent with the other tasks.
Also, do you see value in having an "all" option where it grabs the sizes from all of the databases?
tasks/kb0287.sh
Outdated
| @@ -0,0 +1,18 @@ | |||
| #!/bin/bash | |||
| # Puppet Task Name: KB0287 | |||
| # shellcheck disable=SC2078 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for disabling the shellcheck rather than changing the if statement to comply?
if puppet resource service pe-postgresql | grep -q running
then
echo "pe-postgresql service detected, will continue to run."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to reflect this.
tasks/kb0287.sh
Outdated
| then | ||
| echo "pe-postgresql service detected, will continue to run." | ||
|
|
||
| su - pe-postgres -s /bin/bash -c "/opt/puppetlabs/server/bin/psql -d $dbname -c '\di+;'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there is some missing indentation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bash noob, should be fixed now.
tasks/kb0287.sh
Outdated
| @@ -0,0 +1,18 @@ | |||
| #!/bin/bash | |||
| # Puppet Task Name: KB0287 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on @MartyEwings's email this morning, we should rename this task to include a brief description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
|
@jarretlavallee I have made the suggested changes. I totally agree, it would be nice to have an option to select all tables, so I will add that in a separate PR as I will have to change the KB as well. |
|
Got time today to add the ability to select all tables from all dbs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jarret's comments have been addressed and the all option added. Logic was checked and found sound.
Happy to merge.
| "parameters": { | ||
| "dbname": { | ||
| "description": "The name of the db to connect to", | ||
| "type": "Enum['pe-puppetdb', 'pe-postgres', 'pe-classifier', 'pe-rbac', 'pe-activity', 'pe-orchestrator', 'postgres', all]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the all need to be in single quotes like the other elements in the array?
|
👍 Looks good to me. It is good to merge as long as the enum |
Adding task which will display the selected database's table sizes.