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

fix: Prevent panics when logging HTTP response status in github and gitlab client #4082

Merged
merged 4 commits into from
Dec 22, 2023

Conversation

adkafka
Copy link
Contributor

@adkafka adkafka commented Dec 20, 2023

what

Fixes #4081

why

Wraps any logging that was added from #3876 with an if resp != nil check, to avoid any panics

tests

N/A

references

@adkafka adkafka requested review from a team as code owners December 20, 2023 18:23
@adkafka adkafka requested review from jamengual, lukemassa and nitrocode and removed request for a team December 20, 2023 18:23
@github-actions github-actions bot added go Pull requests that update Go code provider/github provider/gitlab labels Dec 20, 2023
@adkafka adkafka changed the title Prevent panics when logging HTTP response status in github and gitlab client fix: Prevent panics when logging HTTP response status in github and gitlab client Dec 20, 2023
@lukemassa
Copy link
Contributor

I think you missed two:

I found these by running the following:

atlantis % grep -r 'logger.Debug.*resp.StatusCode' server -B1 | grep -v logger.Debug | grep -v 'resp.*nil' | grep server 
server/events/vcs/gitlab_client.go-	_, resp, err := g.Client.AwardEmoji.CreateMergeRequestAwardEmojiOnNote(repo.FullName, pullNum, int(commentID), &gitlab.CreateAwardEmojiOptions{Name: reaction})
server/events/vcs/github_client.go-	repo, resp, err := g.client.Repositories.Get(g.ctx, pull.BaseRepo.Owner, pull.BaseRepo.Name)

I mentioned in #4081 that it might be worth creating a helper function to handle this in a generic way, but I personally am OK with accepting this PR as is since it explicitly and in a straightforward way guards against panics. We should follow up with a more holistic fix (for example, since this approach means that we don't get logs when err != nil, which would likely be useful.)

@adkafka
Copy link
Contributor Author

adkafka commented Dec 20, 2023

Thanks for catching these! I just added fixes for both to this PR. I agree, that there may be better ways to solve this. I saw a simple fix, so I thought it was worth proposing. I'd argue that it is worth merging this to fix things, and we can always follow-up with a refactor if that is desired, but I'll let you guys decide.

Copy link
Member

@chenrui333 chenrui333 left a comment

Choose a reason for hiding this comment

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

lgtm

Fix potential nil pointers. See runatlantis#4081 for context.
Add missing fix for github_client
Add missing fix for gitlab_client
@lukemassa
Copy link
Contributor

/cherry-pick release-0.27

@lukemassa lukemassa merged commit f5d92c6 into runatlantis:main Dec 22, 2023
24 checks passed
@adkafka adkafka deleted the patch-1 branch December 22, 2023 17:02
lukemassa pushed a commit that referenced this pull request Dec 22, 2023
…itlab client (#4082)

Fix potential nil pointers. See #4081 for context.
@GenPage GenPage added the bug Something isn't working label Jan 21, 2024
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
ijames-gc pushed a commit to gocardless/atlantis that referenced this pull request Feb 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working go Pull requests that update Go code provider/github provider/gitlab
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Atlantis v0.27.0 hits nil pointer dereference panic on github client
4 participants