Skip to content

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
Collaborator

💛 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants