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

Package elastic_integration plugin. #15769

Merged
merged 13 commits into from Feb 14, 2024

Conversation

mashhurs
Copy link
Contributor

@mashhurs mashhurs commented Jan 10, 2024

Release notes

[rn:skip]

What does this PR do?

  • Introduces a feature (based on oss-skip flag of the plugin metadata) to exclude plugins in OSS distributions
  • Sets elastic_integration plugin as a default plugin

History

CI fails were due to elastic/logstash-filter-elastic_integration#114

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

Non-user affected.

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

Before merging:

How to test this PR locally

  • Pull this change and run rake artifact:archives_oss (or any of [rpm_oss, deb_oss. docker_oss]) command.
  • Commands generate OSS distributions under build. If we unzip and check the dependencies, we don't include oss-excluded plugins.

NOTE: currently gradle tasks (./gradlew clean assembleOssTarDistribution, ./gradlew clean assembleOssZipDistribution) to build OSS distros are broken, and the reason looks like this issue

Related issues

Use cases

Screenshots

Logs

@mashhurs mashhurs self-assigned this Jan 10, 2024
@@ -61,6 +61,16 @@ namespace "plugin" do
task.reenable # Allow this task to be run again
end

task "install-default-oss" => "bootstrap" do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

review note: I am okay to add [:oss] as an argument and exclude OSS except plugins but I prefered to separate to make clearly visible. When we add an argument to the task, multiple (ex: prepare -> install-default) tasks should carry this argument.

@mashhurs mashhurs marked this pull request as ready for review January 10, 2024 20:45
@mashhurs mashhurs changed the title Exclude plugins feature in OSS distributions. Package elastic_integration plugin. Jan 10, 2024
@yaauie
Copy link
Member

yaauie commented Jan 11, 2024

I think in terms of our dependency graph, it may be better to start with the complete list, and then uninstall/deactivate the non-OSS-licensed plugins. This would ensure that the graphs are otherwise identical.

@@ -378,6 +378,7 @@ namespace "artifact" do

task "generate_build_metadata" do
require 'time'
require 'tempfile'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

review note: when firing this rake task alone, we get uninitialized tempfile error.

@mashhurs
Copy link
Contributor Author

buildkite test this again

@mashhurs
Copy link
Contributor Author

Retried CI, now 🟢

Copy link

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @mashhurs

@mashhurs
Copy link
Contributor Author

@jsvd recap these changes:

  • When removing plugins (bin/logstash-plugin remove {plugin}) we have a Bundler::setup source, it resets all dependency graph. If we have multiple rake tasks running bundle in a sequence (ex: bootstrap, default plugins install) after plugin remove task, we end up getting error says Bundler::GemNotFound: Could not find rubocop-1.60.2... (example failed DRA). Worth noting here that, we hadn't a logic to remove plugin(s) during the distro building.

    • So, the origin issue not with bundle but with Gem::Specification. In case of locally installed gems, unless we fully reload the dependency graph (Gemfile), Gem::Specification doesn't recognize the local gems. That's the reason Bundle::setup was placed. I have changed the logic, in a way that make sure that the plugin in our local gemset. Succeeded DRA
  • Order of the distro build matters: it is weird that if build non-OSS and then OSS (in the artifact:build tasks), maybe because of bundler cache default installed plugins stays same as non-OSS. And I confirmed (excluded some plugins in deb with default_plugin - a_plugin) that it is not due to above plugin remove change, our default plugins were constant so far, we didn't know I guess. I will add a comment if we don't have a better way, I also hate keep such unrelated contextual orders. Wonder if you have any ideas.

puts("[plugin:remove-non-oss-plugins] Removing non-OSS plugins")

LogStash::RakeLib::OSS_EXCLUDED_PLUGINS.each do |plugin|
remove_plugin(plugin)
Copy link
Member

Choose a reason for hiding this comment

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

Just a note that for now this approach works but if in the future any non-OSS plugin has a dependency, we'll have one or two issues to be aware of:

  1. non-OSS may introduce dependencies that will not be used by Logstash
  2. if the non-OSS brings a non-OSS dependency we'll have to remove it as well.

@jsvd jsvd self-requested a review February 13, 2024 16:43
Copy link
Member

@jsvd jsvd left a comment

Choose a reason for hiding this comment

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

LGTM

@mashhurs mashhurs merged commit 3c9db65 into elastic:main Feb 14, 2024
5 checks passed
@mashhurs mashhurs deleted the oss-excluded-plugin-feature branch February 14, 2024 15:10
@mashhurs
Copy link
Contributor Author

@logstashmachine backport 8.13

github-actions bot pushed a commit that referenced this pull request Feb 14, 2024
* Exclude plugins feature in OSS distributions.

* Set elastic_integration plugin default.

* Remove non-OSS plugins after installing default plugins.

* Testing local can't find gem bundler (= 2.3.26) issue.

* Include extract non-OSS plugins logic indocker build operations.

* Only default plugins can be excluded from OSS distros.

* Simplification: instead conditional check, use intersection to make OSS exlucluded plugin list.

* Gem and specification files still stay after removing the plugin. This change removes the stayed files.

* Rename oss-exclude to skip-oss to align namings with other params.

* Make intersection method simpler.

* [Test] Temporary excluding elastic integration plugin from default plugin list.

* Sets elastic_integration plugin default back. When removing locally installed gems, Gem::Specification doesn't recognize the gem. We have Bundle::setup in the removal logic but it is causing an issue when we re-use the bundle.

* Test the build order, remove plugin from cache logic seems invalid since we don't pack the cache.

(cherry picked from commit 3c9db65)
mashhurs added a commit that referenced this pull request Feb 14, 2024
* Exclude plugins feature in OSS distributions.

* Set elastic_integration plugin default.

* Remove non-OSS plugins after installing default plugins.

* Testing local can't find gem bundler (= 2.3.26) issue.

* Include extract non-OSS plugins logic indocker build operations.

* Only default plugins can be excluded from OSS distros.

* Simplification: instead conditional check, use intersection to make OSS exlucluded plugin list.

* Gem and specification files still stay after removing the plugin. This change removes the stayed files.

* Rename oss-exclude to skip-oss to align namings with other params.

* Make intersection method simpler.

* [Test] Temporary excluding elastic integration plugin from default plugin list.

* Sets elastic_integration plugin default back. When removing locally installed gems, Gem::Specification doesn't recognize the gem. We have Bundle::setup in the removal logic but it is causing an issue when we re-use the bundle.

* Test the build order, remove plugin from cache logic seems invalid since we don't pack the cache.

(cherry picked from commit 3c9db65)

Co-authored-by: Mashhur <99575341+mashhurs@users.noreply.github.com>
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.

None yet

5 participants