-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Convert most OSS plugins from integTest to [yaml | java]RestTest or internalClusterTest #59444
Changes from 6 commits
376db42
1e56bb3
424fd5e
5fd84d2
aa96ed8
dfebc85
2be2c71
c603b80
bbc08f9
a9a34d9
5c75412
2fab21b
cc09550
cd76cf4
4a2d639
8682017
4a1d8c8
9414e3d
f2cf390
21cc84a
ebb6703
6147473
05e9421
885c84a
4ea875a
ea705a5
c89b5de
849d19d
ef34668
7b07432
8058400
1832e79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ | |
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
apply plugin: 'elasticsearch.rest-resources' | ||
apply plugin: 'elasticsearch.yaml-rest-test' | ||
|
||
esplugin { | ||
description 'The Japanese (kuromoji) Analysis plugin integrates Lucene kuromoji analysis module into elasticsearch.' | ||
|
@@ -33,6 +33,8 @@ restResources { | |
} | ||
} | ||
|
||
integTest.enabled = false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this being disabled? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need a plan for ripping the creation of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found a need to implement a |
||
|
||
tasks.named("dependencyLicenses").configure { | ||
mapping from: /lucene-.*/, to: 'lucene' | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ | |
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
apply plugin: 'elasticsearch.rest-resources' | ||
apply plugin: 'elasticsearch.yaml-rest-test' | ||
|
||
esplugin { | ||
description 'The Korean (nori) Analysis plugin integrates Lucene nori analysis module into elasticsearch.' | ||
|
@@ -32,6 +32,9 @@ restResources { | |
includeCore '_common', 'indices', 'index', 'search' | ||
} | ||
} | ||
|
||
integTest.enabled = false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. If we need to disable a test, can we have a comment explaining why it is disabled? Someone coming along in the future will have no reason whether this can/should be re-enabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't really disabling a test ... it is to avoid the testing conventions from complaining. I would prefer to tweak the testing conventions instead of a lot of redunant comments. I can do that in a separate PR (and remove these in this PR) |
||
|
||
tasks.named("dependencyLicenses").configure { | ||
mapping from: /lucene-.*/, to: 'lucene' | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,9 +16,8 @@ | |
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
apply plugin: 'elasticsearch.testclusters' | ||
apply plugin: 'elasticsearch.esplugin' | ||
apply plugin: 'elasticsearch.rest-resources' | ||
apply plugin: 'elasticsearch.yaml-rest-test' | ||
|
||
esplugin { | ||
name 'custom-settings' | ||
|
@@ -28,7 +27,9 @@ esplugin { | |
noticeFile rootProject.file('NOTICE.txt') | ||
} | ||
|
||
testClusters.integTest { | ||
testClusters.all { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason this needs to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I figured the best example would be to use |
||
// Adds a setting in the Elasticsearch keystore before running the integration tests | ||
keystore 'custom.secured', 'password' | ||
} | ||
|
||
integTest.enabled = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is needed since the yamlRestRest does not have access to that class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be explicit, applying the signature to
forbiddanApisMain
andforbiddenApisTest
, rather than all CheckForbiddenApis tasks? We are likely to continue adding more source sets in the future, so having configuration apply to all source sets would be problematic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 done
5c75412
(#59444) for the main source set (probably not the extra config noise for the test and internalTestCluster source sets)