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

Schedule strategy for PagerDuty #8

Merged
merged 14 commits into from
Apr 12, 2024
2 changes: 1 addition & 1 deletion Justfile
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ test: generate
go test -v ./...

update-golden: generate
go test -v ./... -test.update-golden
go test -v ./tfrender -test.update-golden

mod:
go mod tidy
Expand Down
86 changes: 75 additions & 11 deletions pager/pagerduty.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package pager
import (
"context"
"fmt"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -66,27 +67,43 @@ func (p *PagerDuty) saveScheduleToDB(ctx context.Context, schedule pagerduty.Sch
}

func (p *PagerDuty) saveLayerToDB(ctx context.Context, schedule pagerduty.Schedule, layer pagerduty.ScheduleLayer) error {
desc := fmt.Sprintf("%s (%s)", schedule.Description, layer.Name)
desc = strings.TrimSpace(desc)

s := store.InsertExtScheduleParams{
ID: schedule.ID + "-" + layer.ID,
Name: schedule.Name + "-" + layer.Name,
Name: schedule.Name + " - " + layer.Name,
Timezone: schedule.TimeZone,

// Add fallback values and override them later if API provides valid information.
Description: fmt.Sprintf("%s (%s)", schedule.Description, layer.Name),
HandoffTime: "11:00",
HandoffDay: "wednesday",
Strategy: "weekly",
Description: desc,
HandoffTime: "11:00:00",
HandoffDay: "wednesday",
Strategy: "weekly",
ShiftDuration: "",
}

// TODO: support custom strategy. For now:
// - any less than "weekly" is considered "daily"
// - any more than "weekly" is considered "weekly" anyway
if layer.RotationTurnLengthSeconds < 60*60*24*7 {
switch layer.RotationTurnLengthSeconds {
case 60 * 60 * 24:
s.Strategy = "daily"
case 60 * 60 * 24 * 7:
s.Strategy = "weekly"
default:
s.Strategy = "custom"
s.ShiftDuration = fmt.Sprintf("PT%dS", layer.RotationTurnLengthSeconds)

now := time.Now()
loc, err := time.LoadLocation(schedule.TimeZone)
if err == nil {
now = now.In(loc)
} else {
console.Warnf("unable to parse timezone '%v', using current machine's local time", schedule.TimeZone)
}
s.StartTime = now.Format(time.RFC3339)
}
virtualStart, err := time.Parse(time.RFC3339, layer.RotationVirtualStart)
if err == nil {
s.HandoffTime = fmt.Sprintf("%02d:%02d", virtualStart.Hour(), virtualStart.Minute())
s.HandoffTime = virtualStart.Format(time.TimeOnly)
s.HandoffDay = strings.ToLower(virtualStart.Weekday().String())
} else {
console.Errorf("unable to parse virtual start time '%v', assuming default values", layer.RotationVirtualStart)
Expand Down Expand Up @@ -127,7 +144,54 @@ func (p *PagerDuty) saveLayerToDB(ctx context.Context, schedule pagerduty.Schedu
}
}

// TODO: add restrictions
for i, restriction := range layer.Restrictions {
switch restriction.Type {
case "daily_restriction":
for day := range 7 {
start, err := time.Parse(time.TimeOnly, restriction.StartTimeOfDay)
if err != nil {
return fmt.Errorf("parsing start time of day '%s': %w", restriction.StartTimeOfDay, err)
}
end := start.Add(time.Duration(restriction.DurationSeconds) * time.Second)

dayStr := strings.ToLower(time.Weekday(day).String())
r := store.InsertExtScheduleRestrictionParams{
ScheduleID: s.ID,
RestrictionIndex: fmt.Sprintf("%d-%d", i, day),
StartDay: dayStr,
EndDay: dayStr,
StartTime: start.Format(time.TimeOnly),
EndTime: end.Format(time.TimeOnly),
}
if err := q.InsertExtScheduleRestriction(ctx, r); err != nil {
return fmt.Errorf("saving daily restriction: %w", err)
}
}
case "weekly_restriction":
start, err := time.Parse(time.TimeOnly, restriction.StartTimeOfDay)
if err != nil {
return fmt.Errorf("parsing start time of day '%s': %w", restriction.StartTimeOfDay, err)
}
// 0000-01-01 is a Saturday, so we need to adjust +1 such that when
// restriction.StartDayOfWeek is 0, it yields Sunday.
start = start.AddDate(0, 0, int(restriction.StartDayOfWeek+1))
end := start.Add(time.Duration(restriction.DurationSeconds) * time.Second)

r := store.InsertExtScheduleRestrictionParams{
ScheduleID: s.ID,
RestrictionIndex: strconv.Itoa(i),
StartDay: strings.ToLower(start.Weekday().String()),
EndDay: strings.ToLower(end.Weekday().String()),
StartTime: start.Format(time.TimeOnly),
EndTime: end.Format(time.TimeOnly),
}
if err := q.InsertExtScheduleRestriction(ctx, r); err != nil {
return fmt.Errorf("saving weekly restriction: %w", err)
}
default:
console.Warnf("Unknown schedule restriction type '%s' for schedule '%s', skipping...\n", restriction.Type, s.ID)
Copy link
Member

@davidcelis davidcelis Apr 12, 2024

Choose a reason for hiding this comment

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

out of curiosity, have you encountered this? i'm guessing they might have a "custom" restriction type that can wrap around day boundaries? in which case, we should be able to support those

Copy link
Member Author

Choose a reason for hiding this comment

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

their API only says daily_restriction and weekly_restriction, nothing else. i added the line just to "fail loudly" so we can learn any unknown behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry that didn't answer your question — no i havent 😅

}
}

return nil
}
Expand Down
51 changes: 45 additions & 6 deletions pager/pagerduty_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@ import (
"net/http/httptest"
"net/url"
"os"
"reflect"
"testing"

"github.com/firehydrant/signals-migrator/pager"
"github.com/firehydrant/signals-migrator/store"
"github.com/gosimple/slug"
)

Expand All @@ -34,6 +36,13 @@ func pagerDutyTestClient(t *testing.T, apiURL string) (context.Context, *pager.P
return ctx, pd
}

func assertDeepEqual[T any](t *testing.T, got, want T) {
t.Helper()
if !reflect.DeepEqual(got, want) {
t.Fatalf("[FAIL]\n got: %+v\nwant: %+v", got, want)
}
}

func TestPagerDuty(t *testing.T) {
ts := pagerDutyHttpServer(t)
ctx, pd := pagerDutyTestClient(t, ts.URL)
Expand All @@ -44,17 +53,47 @@ func TestPagerDuty(t *testing.T) {
t.Fatalf("error loading schedules: %s", err)
}
t.Logf("found %d users", len(u))
// The only assertion so far from this test is that method will not error.
// We need stronger checks for this test to be more useful.
// TODO: verify that the users are accurate.

// Verify that the first user is as expected.
mika := u[0]
expected := &pager.User{
Email: "[email protected]",
Resource: pager.Resource{
ID: "P5A1XH2",
Name: "Mika",
},
}
assertDeepEqual(t, mika, expected)
})

t.Run("LoadSchedules", func(t *testing.T) {
if err := pd.LoadSchedules(ctx); err != nil {
t.Fatalf("error loading schedules: %s", err)
}
// The only assertion so far from this test is that method will not error.
// We need stronger checks for this test to be more useful.
// TODO: verify that the schedules were saved to the database.

// At the moment, this will show "Team ... not found" warning in logs because
// we didn't seed the database with that information. After we refactor the methods
// ListTeams and ListUsers to use database, as LoadTeams and LoadUsers respectively,
// we should expect the warning to go away.
s, err := store.UseQueries(ctx).ListExtSchedules(ctx)
if err != nil {
t.Fatalf("error loading schedules: %s", err)
}
t.Logf("found %d schedules", len(s))

// Verify that the first schedule is as expected.
first := s[0]
expected := store.ExtSchedule{
ID: "P3D7DLW-PC1DX4O",
Name: "Jen - primary - Layer 2",
Description: "(Layer 2)",
Timezone: "America/Los_Angeles",
Strategy: "weekly",
ShiftDuration: "",
StartTime: "",
HandoffTime: "16:00:00",
HandoffDay: "monday",
}
assertDeepEqual(t, first, expected)
})
}
5 changes: 3 additions & 2 deletions pager/testdata/TestPagerDuty/schedules.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,10 @@
"rendered_schedule_entries": [],
"restrictions": [
{
"duration_seconds": 28800,
"type": "weekly_restriction",
"start_time_of_day": "09:00:00",
"type": "daily_restriction"
"duration_seconds": 374400,
"start_day_of_week": 1
Comment on lines +57 to +60
Copy link
Member Author

Choose a reason for hiding this comment

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

add some variation to spice things up

}
],
"rotation_turn_length_seconds": 604800,
Expand Down
18 changes: 9 additions & 9 deletions pager/testdata/TestPagerDuty/teams.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
{
"default_role": "manager",
"description": null,
"html_url": "https://firehydrant-eng.pagerduty.com/teams/PT54U20",
"html_url": "https://acme-inc.pagerduty.com/teams/PT54U20",
Copy link
Member Author

Choose a reason for hiding this comment

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

😬

"id": "PT54U20",
"name": "Jen",
"parent": null,
Expand All @@ -17,7 +17,7 @@
{
"default_role": "manager",
"description": null,
"html_url": "https://firehydrant-eng.pagerduty.com/teams/PO206TE",
"html_url": "https://acme-inc.pagerduty.com/teams/PO206TE",
"id": "PO206TE",
"name": "canary-team",
"parent": null,
Expand All @@ -28,7 +28,7 @@
{
"default_role": "manager",
"description": null,
"html_url": "https://firehydrant-eng.pagerduty.com/teams/PO34CI9",
"html_url": "https://acme-inc.pagerduty.com/teams/PO34CI9",
"id": "PO34CI9",
"name": "CS-Team-Test",
"parent": null,
Expand All @@ -39,7 +39,7 @@
{
"default_role": "manager",
"description": null,
"html_url": "https://firehydrant-eng.pagerduty.com/teams/PEGBTKE",
"html_url": "https://acme-inc.pagerduty.com/teams/PEGBTKE",
"id": "PEGBTKE",
"name": "CS-Test-Alerting",
"parent": null,
Expand All @@ -50,7 +50,7 @@
{
"default_role": "manager",
"description": null,
"html_url": "https://firehydrant-eng.pagerduty.com/teams/P7E9UET",
"html_url": "https://acme-inc.pagerduty.com/teams/P7E9UET",
"id": "P7E9UET",
"name": "caroline-team-test",
"parent": null,
Expand All @@ -61,7 +61,7 @@
{
"default_role": "manager",
"description": null,
"html_url": "https://firehydrant-eng.pagerduty.com/teams/P7A9Q6H",
"html_url": "https://acme-inc.pagerduty.com/teams/P7A9Q6H",
"id": "P7A9Q6H",
"name": "operational-team-test",
"parent": null,
Expand All @@ -72,7 +72,7 @@
{
"default_role": "manager",
"description": null,
"html_url": "https://firehydrant-eng.pagerduty.com/teams/P5PH8KY",
"html_url": "https://acme-inc.pagerduty.com/teams/P5PH8KY",
"id": "P5PH8KY",
"name": "Page Responder Team",
"parent": null,
Expand All @@ -83,7 +83,7 @@
{
"default_role": "manager",
"description": null,
"html_url": "https://firehydrant-eng.pagerduty.com/teams/PV9JOXL",
"html_url": "https://acme-inc.pagerduty.com/teams/PV9JOXL",
"id": "PV9JOXL",
"name": "Service Catalog Team",
"parent": null,
Expand All @@ -94,7 +94,7 @@
{
"default_role": "manager",
"description": null,
"html_url": "https://firehydrant-eng.pagerduty.com/teams/PD2F80U",
"html_url": "https://acme-inc.pagerduty.com/teams/PD2F80U",
"id": "PD2F80U",
"name": "Jack Team",
"parent": null,
Expand Down
14 changes: 9 additions & 5 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ This can be used to import resources from legacy alerting providers into Signals
## Supported providers

### Alerting

- PagerDuty
- VictorOps
- OpsGenie
Expand Down Expand Up @@ -37,11 +38,14 @@ Afterwards, the tool will generate the mapping appropriately, handling de-duplic

## Feature roadmap

- [x] Importing users
- [x] Importing teams and members
- [ ] Pre-create a default escalation policy
- [ ] Import scheduling strategy
- [ ] Pre-create scheduling strategy
| | PagerDuty | VictorOps | OpsGenie |
| --- | --- | --- | --- |
| Import users | :white_check_mark: | :white_check_mark: | :white_check_mark: |
| Import teams and members | :white_check_mark: | :white_check_mark: | :white_check_mark: |
| Import escalation policies | :x: | :x: | :x: |
| Import scheduling strategy | :white_check_mark: | :x: | :x: |


- [ ] Getting transposer URLs (e.g. Datadog) to the team data resource or a Signals ingest URL data resource
- [ ] Support for importing escalation policies
- [ ] Auto-run `terraform apply` for users who would not manage their organization with Terraform after importing
Expand Down
25 changes: 18 additions & 7 deletions store/models.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion store/queries.sql
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,13 @@ WHERE ext_teams.id = ?;
SELECT * FROM ext_schedules;

-- name: InsertExtSchedule :exec
INSERT INTO ext_schedules (id, name, description, timezone, strategy, handoff_time, handoff_day) VALUES (?, ?, ?, ?, ?, ?, ?);
INSERT INTO ext_schedules (id, name, description, timezone, strategy, shift_duration, start_time, handoff_time, handoff_day) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?);

-- name: ListExtScheduleRestrictionsByExtScheduleID :many
SELECT * FROM ext_schedule_restrictions WHERE schedule_id = ?;

-- name: InsertExtScheduleRestriction :exec
INSERT INTO ext_schedule_restrictions (schedule_id, restriction_index, start_time, start_day, end_time, end_day) VALUES (?, ?, ?, ?, ?, ?);

-- name: ListFhTeamsByExtScheduleID :many
SELECT ext_teams.*, fh_teams.name as fh_team_name, fh_teams.slug as fh_team_slug FROM ext_schedule_teams
Expand Down
Loading