Puppet » Puppet Dashboard

I’m making a bunch of documentation changes in the same branch since they’ll all probably touch the README.markdown file. The commit pertinent to this ticket is 55d4ece4f0b50111ce42eb3f39063e4b1955c729

Matt:

Close, but here’s a few changes:

README:

1. Format code, commands and keywords like `rake` with backticks.
2. I don’t think the “Prune” header’s needed, just have the text continue after the introduction to cleanup.
3. Maybe say “For example” in the text right before the command.

prune_reports.rake

1. Move the conversion to an integer to the code setting the “upto” variable.
2. Add some kind of guard to ensure that a variable was provided, because if a blank string or a word gets through, #to_i will return a 0 and this will make the task delete everything.
3. Improve the task #desc to explain that it’ll show help if run without args.

4. Consolidate all the error checks (is ‘unit’ set? is ‘upto’ set? is ‘unit’ valid) so you can print nice, multi-line documentation and examples explaining what’s wrong, how to use the command, etc. I’m thinking something like:

   ERROR: Invalid ‘unit’ specified. Valid units are: foo, bar baz

   USAGE: rake upto=UPTO unit=UNIT

   OPTIONS:

   • upto: …
   • unit: … {and valid units}

   EXAMPLES: …

New commit on the end of the branch with the changes.

I had already backticked things in another commit.

rake reports:prune is now friendlier. There had been a guard before to make sure upto was an integer through a regex. Longer usage message plus more targetted errors. I also print the time you’re deleting before which was both helpful in debugging my changes plus nice to know for the user running the command.

I think this is the last bit of feedback on the documentation branch I needed to respond to.

Matt:

Good changes, I added some additional improvements. I also found a serious bug in this code from before you touched it and have expanded the scope of the ticket to resolve it as well.

Please review my changes at:

http://github.com/igal/puppet-dashboard/tree/bug-4470-prune_reports

[#4470] Fix which reports to prune, improve documentation and refactor.

Reports were incorrectly pruned by the 'created_at' field, which is the
time these records were imported into Dashboard. They're now pruned by the
'time' field, which is the time they were generated by the client.

The time used as the pruning cutoff was specified as a raw SQL string.
It's now kept as a Ruby Time object so ActiveRecord can generate
appropriate, database-specific SQL.

Extra query was done to count the records to delete, now this count is
retrieved from the single #delete_all command.

Error conditions had overlapping logic, now they're consolidated.

Documentation emitted unwanted leading and trailing spaces, no separator
between error message and documentation, and lacked colons to separate
descriptions from units.

I sent a reply to this ticket in email, but the ticket hasn’t updated in the last 5 minutes and I realized that the replying doesn’t assign the ticket back to Igal, so I’m assigning the ticket here, and hopefully the comment from email will show up soon. I’ll check back in to make sure that happens.

Hmm, the email is taking forever to update the ticket. I’m copy pasting it in here.

Igal, Good catch. I see you filed another ticket to take care of the other places in dashboard where we make the created_at vs time field mistake too.

There were a few changes you made that didn’t make sense to me:

• upto: How far to prune as an integer
• upto: Number of units to keep prune up to.

I had trouble thinking of the best wording for this, but I don’t think the new wording makes a lot of sense.

Maybe ‘Number of units back in time to keep reports’?

I also thought the error addition was easier to read as a consistent looking set of 6 lines than a 13 line set of nested conditionals, but don’t care that much.

Pushed another change to the docs, had it reviewed in person by Matt.

Will merge later with the rest of the documentation-related tickets.