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

Correct checking instance type of get data response #15554

Merged
merged 6 commits into from
Aug 16, 2023

Conversation

vivek-datadog
Copy link
Contributor

@vivek-datadog vivek-datadog commented Aug 11, 2023

What does this PR do?

_get_data generic exception handler was incorrectly checking for resp variable which would always result in False when the status code is not in successful range

I believe the intent was to ensure the presence of resp variable to avoid an exception inside the generic exception handler

As a result, a critical service check was sent whenever http request in _get_data fails leading to support ticket AGENT-10055

This change ensures that the correct existence of resp variable and the relevant status code, so that auth error is raised as per expectations

Motivation

AGENT-10055

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Changelog entries must be created for modifications to shipped code
  • Add the qa/skip-qa label if the PR doesn't need to be tested during QA.

@github-actions
Copy link

github-actions bot commented Aug 11, 2023

Test Results

     24 files       24 suites   6m 58s ⏱️
   121 tests    118 ✔️   3 💤 0
1 464 runs  1 374 ✔️ 90 💤 0

Results for commit cf6bfa2.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Merging #15554 (f87b658) into master (8dd02f3) will increase coverage by 0.34%.
The diff coverage is 100.00%.

❗ Current head f87b658 differs from pull request most recent head cf6bfa2. Consider uploading reports for the commit cf6bfa2 to get more accurate results

Flag Coverage Δ
elastic 93.75% <100.00%> (+0.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@vivek-datadog vivek-datadog marked this pull request as ready for review August 11, 2023 15:22
@vivek-datadog vivek-datadog requested review from a team as code owners August 11, 2023 15:22
brett0000FF
brett0000FF previously approved these changes Aug 11, 2023
elastic/CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@FlorentClarret FlorentClarret left a comment

Choose a reason for hiding this comment

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

Look good, just left one question 🙂

elastic/tests/test_unit.py Outdated Show resolved Hide resolved
elastic/CHANGELOG.md Outdated Show resolved Hide resolved
brett0000FF
brett0000FF previously approved these changes Aug 14, 2023
Co-authored-by: Ofek Lev <ofekmeister@gmail.com>
@vivek-datadog vivek-datadog changed the title [AGENT-10055] Correct checking instance type of get data response Correct checking instance type of get data response Aug 14, 2023
ofek
ofek previously approved these changes Aug 14, 2023
@vivek-datadog vivek-datadog merged commit 8726262 into master Aug 16, 2023
31 checks passed
@vivek-datadog vivek-datadog deleted the vivek-datadog/AGENT-10055_ branch August 16, 2023 09:05
@yzhan289 yzhan289 mentioned this pull request Aug 21, 2023
3 tasks
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.

None yet

4 participants