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.

Sign up for GitHub

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

Jump to bottom

[Integration testing] Remove AltErr and only expect 400 from ES bulk API #4012

Merged
merged 2 commits into from
Oct 22, 2024
Merged

[Integration testing] Remove AltErr and only expect 400 from ES bulk API #4012

merged 2 commits into from
Oct 22, 2024

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Oct 16, 2024 •
edited
Loading

What is the problem this PR solves?

This PR expects only a 400 HTTP response code from the Elasticsearch bulk API in integration tests when invalid index name is specified. Concretely, it removes the AltErr field temporarily introduced in #4001 which was allowing the tests to accept an alternate error from Elasticsearch.

Related: elastic/elasticsearch#114869

@ycombinator ycombinator added cleanup Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team labels Oct 16, 2024
@ycombinator ycombinator requested a review from a team as a code owner October 16, 2024 18:41
@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 16, 2024

This pull request does not have a backport label. Could you fix it @ycombinator? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-./d./d is the label to automatically backport to the 8./d branch. /d is the digit

@mergify Mergify
Copy link
Contributor

mergify bot commented Oct 16, 2024

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

@mergify mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 16, 2024
@ycombinator ycombinator mentioned this pull request Oct 16, 2024
Merged
blakerouse
blakerouse approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@blakerouse blakerouse left a comment

Choose a reason for hiding this comment

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

+1

@jlind23
Copy link
Contributor

jlind23 commented Oct 18, 2024

Looks like we have a failure on FAIL: TestBulkCreate/Invalid_utf-8 (0.26s)
Shall we change this expected code to 400 instead -

fleet-server/internal/pkg/bulk/bulk_integration_test.go

Line 73 in deefa2d

Status: 500,
?

@ycombinator ycombinator mentioned this pull request Oct 19, 2024
Closed
8 tasks
@ycombinator
Copy link
Contributor Author

Looks like we have a failure on FAIL: TestBulkCreate/Invalid_utf-8 (0.26s) Shall we change this expected code to 400 instead -

fleet-server/internal/pkg/bulk/bulk_integration_test.go

Line 73 in deefa2d

Status: 500,

?

This change is already part of this PR. So I'm not sure why this is test is failing. Investigating...

@elastic-sonarqube Elastic SonarQube
Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube

@ycombinator ycombinator enabled auto-merge (squash) October 22, 2024 00:29
@ycombinator ycombinator merged commit 1ae9ab6 into elastic:main Oct 22, 2024
8 checks passed
mergify bot pushed a commit that referenced this pull request Oct 22, 2024
@ycombinator @mergify
[Integration testing] Remove AltErr and only expect 400 from ES bulk …
0115e46 
…API (#4012)

* Remove AltErr

* Fixed error type

(cherry picked from commit 1ae9ab6)

# Conflicts:
#	internal/pkg/bulk/bulk_integration_test.go
@mergify mergify bot mentioned this pull request Oct 22, 2024
Merged
@pierrehilbert
Copy link
Contributor

Should we backport this to 8.16 also?

@ycombinator
Copy link
Contributor Author

Should we backport this to 8.16 also?

Doesn't look like the ES fix was backported to 8.16, so I don't think this PR should be backported to 8.16 either.

@ycombinator ycombinator deleted the bulk-integ-test-es-resp-code branch October 22, 2024 12:09
ycombinator added a commit that referenced this pull request Oct 29, 2024
@ycombinator
[Integration testing] Remove AltErr and only expect 400 from ES bulk …
1b4165c 
…API (#4012)

* Remove AltErr

* Fixed error type

(cherry picked from commit 1ae9ab6)

# Conflicts:
#	internal/pkg/bulk/bulk_integration_test.go
ycombinator added a commit that referenced this pull request Oct 30, 2024
@mergify @ycombinator @jlind23
[8.x](backport #4012) [Integration testing] Remove AltErr and only ex…
457a697 
…pect 400 from ES bulk API (#4030)

* [Integration testing] Remove AltErr and only expect 400 from ES bulk API (#4012)

* Remove AltErr

* Fixed error type

(cherry picked from commit 1ae9ab6)

# Conflicts:
#	internal/pkg/bulk/bulk_integration_test.go

* Update bulk_integration_test.go

Fix mergify mess

* Printing out filename for debugging

* Fixing up conflicts

* Trigger Build

---------

Co-authored-by: Shaunak Kashyap <[email protected]>
Co-authored-by: Julien Lind <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michel-laterman michel-laterman michel-laterman left review comments

@swiatekm swiatekm swiatekm approved these changes

@blakerouse blakerouse blakerouse approved these changes

Assignees
No one assigned
Labels
backport-8.x Automated backport to the 8.x branch with mergify cleanup Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ycombinator @jlind23 @pierrehilbert @blakerouse @michel-laterman @swiatekm