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

pager/og: safer teamID retrieval #39

Merged
merged 3 commits into from
May 14, 2024
Merged

pager/og: safer teamID retrieval #39

merged 3 commits into from
May 14, 2024

Conversation

wilsonehusin
Copy link
Member

Received a user report error on this:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x102bf49c8]
github.com/firehydrant/signals-migrator/pager.(*Opsgenie).saveEscalationPolicyToDB(0x14000412160, {0x102f46518, 0x14000276b70}, {{0x1400016e240, 0x24}, {0x1400002b038, 0x15}, {0x0, 0x0}, {0x14000165860, ...}, ...})
	~/signals-migrator/pager/opsgenie.go:363 +0x128
github.com/firehydrant/signals-migrator/pager.(*Opsgenie).LoadEscalationPolicies(0x14000412160, {0x102f46518, 0x14000276b70})

Seems like Opsgenie doesn't guarantee OwnerTeam to be present.

for _, u := range unmatched[i:] {
for _, u := range toImport[i:] {
Copy link
Member Author

Choose a reason for hiding this comment

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

this bug caused us to provision everyone regardless of the people who were selected 🙃 i'm going to follow up with a refactor + test. this is a quick fix for now.

@@ -211,6 +212,8 @@ func (c *Client) CreateUser(ctx context.Context, u SCIMUser) (*store.FhUser, err
return nil, fmt.Errorf("creating user: %w", err)
}
if resp.StatusCode != http.StatusCreated {
body, _ := io.ReadAll(resp.Body)
console.Errorf("unexpected status code %d: %s\n", resp.StatusCode, body)
Copy link
Member Author

Choose a reason for hiding this comment

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

log more information when response code isn't expected — this should help users get better information and can help us figure out where to look.

if policy.Repeat != nil {
repeatLimit = int64(policy.Repeat.Count)
repeatInterval.Valid = true
repeatInterval.String = fmt.Sprintf("PT%dM", policy.Repeat.WaitInterval)
}
teamID := sql.NullString{}
if policy.OwnerTeam != nil {
Copy link
Member Author

Choose a reason for hiding this comment

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

this is apparently nil-able, which would cause panic

Copy link
Contributor

Choose a reason for hiding this comment

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

Why? And how? But these are things we're not meant to know I guess and we just accept them so here we are.

@wilsonehusin wilsonehusin requested a review from AlexisJasso May 14, 2024 20:55
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.

Seems good. Let's do it.

@wilsonehusin wilsonehusin merged commit cb69282 into main May 14, 2024
4 checks passed
@wilsonehusin wilsonehusin deleted the og-team-id branch May 14, 2024 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants