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 flaky test for legacy authorization #87642

Merged
merged 6 commits into from
Jan 15, 2021
Merged

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Jan 7, 2021

Resolves #87010.
Resolves #87098.
Resolves #86952.

The flaky test #87010 uncovered a single occurrence of two things:

  1. Exceeding the retryIfConflicts attempts
  2. Authorization allowing to updateApiKey when it should have been unauthorized

In this PR, I've increased the retryIfConflicts attempts from 1 to 2 for the special case that does happen. For the authorization allowing the updateApiKey call, I couldn't reproduce this, and it could have been fixed already with a PR between then and now. Due to not being able to reproduce and discover the underlying cause, I cleaned up some code that no longer seemed necessary in the updateApiKey function. I hope this may help or resolve the flaky scenario.

Flaky test runs:

@mikecote mikecote added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.12.0 labels Jan 7, 2021
@mikecote mikecote self-assigned this Jan 7, 2021
@mikecote mikecote changed the title Fix legacy authorization flaky test Fix flaky test for legacy authorization Jan 7, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@mikecote mikecote marked this pull request as ready for review January 13, 2021 20:00
@mikecote mikecote requested a review from a team as a code owner January 13, 2021 20:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM, does seem like we had some unnecessary leftover there.
Thanks for cleaning that up.

@mikecote mikecote merged commit a0b787c into elastic:master Jan 15, 2021
mikecote added a commit to mikecote/kibana that referenced this pull request Jan 15, 2021
* Unskip test

* Increase attempts to 2 for retryIfConflicts

* Cleanup authorization for updateApiKey
mikecote added a commit that referenced this pull request Jan 15, 2021
* Unskip test

* Increase attempts to 2 for retryIfConflicts

* Cleanup authorization for updateApiKey
mikecote added a commit to mikecote/kibana that referenced this pull request Jan 26, 2021
* Unskip test

* Increase attempts to 2 for retryIfConflicts

* Cleanup authorization for updateApiKey
mikecote added a commit that referenced this pull request Jan 26, 2021
* Unskip test

* Increase attempts to 2 for retryIfConflicts

* Cleanup authorization for updateApiKey
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment