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

Resolve Prometheus dependency for IT on non-Linux OS. #152

Closed
wants to merge 4 commits into from
Closed

Resolve Prometheus dependency for IT on non-Linux OS. #152

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fbfd013
Resolve Prometheus dependency for IT on non-Linux OS.
Yury-Fridlyand Nov 1, 2022
f0a3bdd
Reorder imports.
Yury-Fridlyand Nov 1, 2022
82942fa
Add code comments.
Yury-Fridlyand Nov 3, 2022
45d97bb
Address PR feedback.
Yury-Fridlyand Nov 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions doctest/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
* SPDX-License-Identifier: Apache-2.0
*/


import java.util.concurrent.Callable
import org.gradle.nativeplatform.platform.internal.DefaultNativePlatform
MaxKsyunz marked this conversation as resolved.
Show resolved Hide resolved
import org.opensearch.gradle.testclusters.RunTask

plugins {
Expand Down Expand Up @@ -31,6 +33,7 @@ task startPrometheus(type: SpawnProcessTask) {
download.run {
src 'https://github.com/prometheus/prometheus/releases/download/v2.39.1/prometheus-2.39.1.linux-amd64.tar.gz'
dest new File("$projectDir/bin", 'prometheus.tar.gz')
overwrite false
}
copy {
from tarTree("$projectDir/bin/prometheus.tar.gz")
Expand Down Expand Up @@ -86,9 +89,17 @@ task stopPrometheus() {

stopPrometheus.mustRunAfter startPrometheus
doctest.dependsOn startOpenSearch
startOpenSearch.dependsOn startPrometheus
doctest.finalizedBy stopOpenSearch
stopOpenSearch.finalizedBy stopPrometheus

// OpenSearch designed to run on Linux only in production, so main CI workflows are on Linux too.
// As of #977 gradle workflows which depend on Prometheus use hardcoded Linux version of it.
// Auxiliary workflows and developers who use other OS should be able to run tests as well,
// so this trick unblocks them to run doctests. Note: Prometheus related tests would fail.
if (DefaultNativePlatform.currentOperatingSystem.isLinux()) {
Copy link

Choose a reason for hiding this comment

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

Thank you!
Can you include a comment saying why we don't run prometheus on non-Linux systems.

Copy link
Author

Choose a reason for hiding this comment

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

I'll be happy to know why...

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 82942fa.

startOpenSearch.dependsOn startPrometheus
stopOpenSearch.finalizedBy stopPrometheus
}

build.dependsOn doctest
clean.dependsOn(cleanBootstrap)

Expand Down
12 changes: 10 additions & 2 deletions integ-test/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ task startPrometheus(type: SpawnProcessTask) {
download.run {
src 'https://github.com/prometheus/prometheus/releases/download/v2.39.1/prometheus-2.39.1.linux-amd64.tar.gz'
dest new File("$projectDir/bin", 'prometheus.tar.gz')
overwrite false
}
copy {
from tarTree("$projectDir/bin/prometheus.tar.gz")
Expand All @@ -135,8 +136,15 @@ stopPrometheus.mustRunAfter startPrometheus
// Run PPL ITs and new, legacy and comparison SQL ITs with new SQL engine enabled
integTest {
dependsOn ':opensearch-sql-plugin:bundlePlugin'
dependsOn startPrometheus
finalizedBy stopPrometheus

// OpenSearch designed to run on Linux only in production, so main CI workflows are on Linux too.
// As of #977 gradle workflows which depend on Prometheus use hardcoded Linux version of it.
// Auxiliary workflows and developers who use other OS should be able to run tests as well,
// so this trick unblocks them to run ITs. Note: Prometheus related tests would be skipped.
if (DefaultNativePlatform.currentOperatingSystem.isLinux()) {
Copy link

Choose a reason for hiding this comment

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

Same here. Maybe a comment why we don't run this for non-Linux.
Someone might ask why don't we support promethius for all OSes

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in 82942fa.

dependsOn startPrometheus
finalizedBy stopPrometheus
}

systemProperty 'tests.security.manager', 'false'
systemProperty('project.root', project.projectDir.absolutePath)
Expand Down
3 changes: 3 additions & 0 deletions integ-test/src/test/java/org/opensearch/sql/legacy/OpenSearchSQLRestTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ protected RestClient buildClient(Settings settings, HttpHost[] hosts) throws IOE
}

protected static void wipeAllOpenSearchIndices() throws IOException {
if (client() == null) {
Copy link

Choose a reason for hiding this comment

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

feels like this check should be moved to RestIntegTestCase::cleanUpIndices and SQLIntegTestCase::cleanUpIndices (those classes already check for client() on setup)

Copy link
Author

Choose a reason for hiding this comment

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

45d97bb

MaxKsyunz marked this conversation as resolved.
Show resolved Hide resolved
return;
}
// include all the indices, included hidden indices.
// https://www.elastic.co/guide/en/elasticsearch/reference/current/cat-indices.html#cat-indices-api-query-params
Response response = client().performRequest(new Request("GET", "/_cat/indices?format=json&expand_wildcards=all"));
Expand Down
14 changes: 14 additions & 0 deletions integ-test/src/test/java/org/opensearch/sql/ppl/ShowCatalogsCommandIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,25 @@
import static org.opensearch.sql.util.MatcherUtils.verifyDataRows;

import java.io.IOException;
import org.apache.commons.lang3.SystemUtils;
import org.json.JSONObject;
import org.junit.Assume;
import org.junit.BeforeClass;
import org.junit.jupiter.api.Test;

public class ShowCatalogsCommandIT extends PPLIntegTestCase {

/**
* OpenSearch designed to run on Linux only in production, so main CI workflows are on Linux too.
* As of #977 gradle workflows which depend on Prometheus use hardcoded Linux version of it.
* Auxiliary workflows and developers who use other OS should be able to run tests as well,
* so this trick skips Prometheus related tests to avoid failures.
*/
@BeforeClass
public static void checkOs() {
Assume.assumeTrue(SystemUtils.IS_OS_LINUX);
}

@Test
public void testShowCatalogsCommands() throws IOException {
JSONObject result = executeQuery("show catalogs");
Expand Down