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

PDE-1852: services as team, use Load* methods #18

Merged
merged 9 commits into from
Apr 22, 2024

Conversation

wilsonehusin
Copy link
Member

@wilsonehusin wilsonehusin commented Apr 22, 2024

  • Implement "group team" in PagerDuty
  • Implement LoadUsers / LoadTeams / LoadTeamMembers in PagerDuty
  • Implement Load* methods for Opsgenie
  • Port over implementation of Load* to cmd/import.go

This ended up being bigger changeset than I had planned, but taking advantage the fact that there is no work in flight and I could do a proper refactor, I went ahead and ported over the old implementations of fetching users, teams, and members to use database properly.

That being said, don't let the delta of lines changed fool you — most of them are test data 😄

filename, err := url.JoinPath("testdata", baseTestDir, "apiserver", slug.Make(r.URL.Path)+".json")
urlPath := r.URL.Path
if r.URL.RawQuery != "" {
urlPath += "?" + r.URL.RawQuery
Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out, this is needed to support pagination 😅

log.Printf("using db file: %s", f)

db, err := sql.Open("sqlite", f)
func NewStore(dsn string) *Store {
Copy link
Member Author

Choose a reason for hiding this comment

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

NewStore at this point is identical to store/open.go, but this feels less critical to refactor now, so I'm leaving it as is for later.

Comment on lines +32 to +41
is_group INTEGER NOT NULL DEFAULT 0,
to_import INTEGER NOT NULL DEFAULT 0
) STRICT;

CREATE TABLE IF NOT EXISTS ext_team_groups (
group_team_id TEXT NOT NULL,
member_team_id TEXT NOT NULL,
PRIMARY KEY (group_team_id, member_team_id),
FOREIGN KEY (group_team_id) REFERENCES ext_teams(id) ON DELETE CASCADE,
FOREIGN KEY (member_team_id) REFERENCES ext_teams(id) ON DELETE CASCADE
Copy link
Member Author

Choose a reason for hiding this comment

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

ext_team_groups stores groups which contain other groups. PagerDuty Services are entered into ext_teams table just like any other teams. However, it is marked as a "team group". Specifically, PagerDuty service would have group_team_id pointing to the Service ID, while the member_team_id points to the real team linked to the service.

If there is already ext_team_groups, why would we need is_group still on ext_teams table?

I initially thought is_group is no longer necessary as it can be inferred from ext_team_groups, but then I realized that a service without any teams associated to it is a valid service which may have escalation policies, etc. As such, we need explicit mark whether the team is a group or not on the ext_teams table.

@@ -17,7 +17,11 @@ import (

func withTestDB(t *testing.T) context.Context {
t.Helper()
ctx := store.WithContext(context.Background())

f := filepath.Join(t.TempDir(), slug.Make(t.Name())+".db")
Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we not use memory store?

Making multiple calls to sql.Open from the same thread yields the same database. This causes problematic collisions when tests are run with t.Parallel().

@wilsonehusin wilsonehusin marked this pull request as ready for review April 22, 2024 21:28
Copy link
Contributor

@AlexisJasso AlexisJasso left a comment

Choose a reason for hiding this comment

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

Yep. This is fine. As noted, most of the lines are test data and there's a significant amount of code being moved around also (which is nice because logically I'd think about users and teams before schedules anyway).

@wilsonehusin wilsonehusin merged commit 9306390 into main Apr 22, 2024
4 checks passed
@wilsonehusin wilsonehusin deleted the wh/pde-1852-service-as-team branch April 22, 2024 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants