Skip to content
This repository has been archived by the owner on Apr 8, 2021. It is now read-only.

updated schema to include more escalation policy and schedule data #10

Merged
merged 2 commits into from Sep 6, 2017

Conversation

amyngyn
Copy link
Contributor

@amyngyn amyngyn commented Sep 5, 2017

Summary

Pull more data from the PagerDuty v1 API. The schema now includes escalation rules and which users are in each schedule. The new tables are: escalation_rules, escalation_rule_users, escalation_rule_schedules, and user_schedule. escalation_policies also now includes a column for how many times it loops.

Motivation

We want to use this data to do more analysis on our use of PD.

Test plan

Ran on local machine and wrote SQL queries for sanity testing.

Rollout/monitoring/revert plan

None

Reviewer expectation

Comments on the schema choices would be helpful, because I wasn't sure if I was representing the PD data with the right SQL idioms.

Copy link

@asf-stripe asf-stripe left a comment

Choose a reason for hiding this comment

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

Some cosmetic / comment comments, but this looks good otherwise!

pd2pg.rb Outdated
"schedules/#{s["id"]}/users",
{ since: Time.now.strftime("%Y-%m-%d") },
false) {
|u| convert_user_schedule(u, s["id"])

Choose a reason for hiding this comment

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

Please use do |u| syntax for multiple-line blocks (with the |u| on the same line as the do); it gets really confusing to read otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😭

if should_log
log("refresh_bulk.update", table_name: table_name, total: records.length)
end
database_update(table_name, records)

Choose a reason for hiding this comment

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

with the addition of this, is the Yields each API value to a block above still true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It's kind of hard to see, but the database_update method is code I copypasted from the bottom of this method into its own method. So it's pretty much the same behavior, but "decomposed". The yield is still used!

@amyngyn
Copy link
Contributor Author

amyngyn commented Sep 5, 2017

Done addressing comments!

Copy link

@asf-stripe asf-stripe left a comment

Choose a reason for hiding this comment

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

Great! \o/

@an-stripe
Copy link
Contributor

Can I get permission on this repo to merge this? (Or can you merge this?)

@an-stripe an-stripe merged commit 1fdf825 into stripe-archive:master Sep 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants