Skip to content

qa: fix integration by including built gems#17165

Closed
yaauie wants to merge 1 commit intoelastic:mainfrom
yaauie:qa-check-in-built-gem-fixtures
Closed

qa: fix integration by including built gems#17165
yaauie wants to merge 1 commit intoelastic:mainfrom
yaauie:qa-check-in-built-gem-fixtures

Conversation

@yaauie
Copy link
Member

@yaauie yaauie commented Feb 27, 2025

Release notes

[rn:skip]

What does this PR do?

Checks in the gems that are needed for plugin removal integration tests introduced in #17030 and backported to 8.x and 9.0

This is the result of invoking the previouly-checked-in script:

qa/integration/fixtures/plugins/generate-gems.sh

Why is it important/What is the impact to the user?

Build fix.

Checklist

  • My code follows the style guidelines of this project
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files (and/or docker env variables)
  • [ ] I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

Observe the buildkite integration tests for cli/remove_spec succeeding.

This is the result of invoking the previouly-checked-in script:

~~~
qa/integration/fixtures/plugins/generate-gems.sh
~~~
@yaauie yaauie added the tests label Feb 27, 2025
@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
No data about Duplication

See analysis details on SonarQube

@elasticmachine
Copy link

💛 Build succeeded, but was flaky

Failed CI Steps

@yaauie yaauie requested a review from donoghuc February 27, 2025 15:06
Copy link
Member

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

Instead of checking in compiled gems, should we just invoke https://github.com/elastic/logstash/blob/main/qa/integration/fixtures/plugins/generate-gems.sh as part of test setup?

@yaauie
Copy link
Member Author

yaauie commented Feb 27, 2025

OK. Integration tests weren't failing because buildkite has host_os == linux and the relevant specs were in an inactive else clause 🤦🏼‍♂️ -> #17171

Copy link
Member

@donoghuc donoghuc left a comment

Choose a reason for hiding this comment

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

I dont think the tests that depend on this are actually being run in CI when they should? For example: https://buildkite.com/elastic/logstash-exhaustive-tests-pipeline/builds/1386#01953de7-7260-4b11-91aa-721dece635d9

CLI > logstash-plugin remove
--
  | without internet connection (linux seccomp wrapper)
  | when other plugins depends on this plugin
  | expunging(/buildkite/builds/bk-agent-prod-k8s-1740500368221313042/elastic/logstash-exhaustive-tests-pipeline/build/qa-fixture/logstash-9.1.0-SNAPSHOT)
  | expanding(/buildkite/builds/bk-agent-prod-k8s-1740500368221313042/elastic/logstash-exhaustive-tests-pipeline/build/logstash-9.1.0-SNAPSHOT.tar.gz)
  | Using /buildkite/builds/bk-agent-prod-k8s-1740500368221313042/elastic/logstash-exhaustive-tests-pipeline/build/qa-fixture/logstash-9.1.0-SNAPSHOT as LS_HOME
  | Setting up services
  | Setting up logstash service
  | Setup script not found for logstash
  | logstash service setup complete
  |  
  | rm -f offline offline.o
  | cc    -c -o offline.o offline.c
  | cc -o offline offline.o

I think the tests are in a

context "when removing plugins and all plugins that depend on them" do
conditional https://buildkite.com/elastic/logstash-exhaustive-tests-pipeline/builds/1386#01953de7-7260-4b11-91aa-721dece635d9 . I think the conditional is wrong, I think we should run the tests in the else block regardless of OS and ONLY run the tests in the if block if we are on linux. I think the way its structured now is on linux we ONLY run the tests in the if block, but we actually want everything in that file.

@yaauie
Copy link
Member Author

yaauie commented Feb 27, 2025

I merged #17171, which has the alternate fix.

@yaauie yaauie closed this Feb 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3 participants