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

Setup keystore during integration tests #22966

Merged
merged 12 commits into from
Mar 20, 2017

Conversation

Tim-Brooks
Copy link
Contributor

This commit creates a keystore and adds settings to it during the
cluster formation for integration tests. Users can define a
keyStoreSetting in build files for settings that need to be placed in
the keystore.

@s1monw
Copy link
Contributor

s1monw commented Feb 13, 2017

this LGTM in general but I'd like to see a test using it even if it's just for the f*** of it.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

This looks good, I left some suggestions on naming.

@@ -306,6 +309,32 @@ class ClusterFormationTasks {
}
}

/** Adds a task to create keystore */
static Task configureCreateKeyStoreTask(String name, Project project, Task setup, NodeInfo node) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you make this a no-op when the keystoreSettings are empty?

@@ -125,6 +125,8 @@ class ClusterConfiguration {

Map<String, Object> settings = new HashMap<>()

Map<String, String> keyStoreSetting = new HashMap<>()
Copy link
Member

Choose a reason for hiding this comment

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

plural please? Also, just for consistency with the code in ES, can we use all lower case for keystore, so keystoreSettings?

}
}

/** Adds tasks to add to keystore */
Copy link
Member

Choose a reason for hiding this comment

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

"to add settings to the keystore"

}

/** Adds tasks to add to keystore */
static Task configureAddKeyStoreTasks(Task parent, Project project, Task setup, NodeInfo node) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe call this configureAddKeystoreSettingTasks

/** Adds a task to create keystore */
static Task configureCreateKeyStoreTask(String name, Project project, Task setup, NodeInfo node) {
File esKeyStoreUtil = Paths.get(node.homeDir.toString(), "bin/" + "elasticsearch-keystore").toFile()
Task createKeyStore = project.tasks.create(name: name, type: LoggedExec, dependsOn: setup) {
Copy link
Member

Choose a reason for hiding this comment

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

One more thing: you should use configureExecTask so this will work correctly on windows.

Copy link
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM

@Tim-Brooks
Copy link
Contributor Author

Thanks. I had been waiting for CI to be up and to test it again. And it just passed, so I am merging this.

@Tim-Brooks Tim-Brooks merged commit 15a5d1d into elastic:master Mar 20, 2017
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Mar 22, 2017
* master:
  Fix typo in allocation explain API docs
  Add unit tests for ReverseNestedAggregator (elastic#23651)
  Revert "Revert "Build: Upgrade min gradle to 3.3 (elastic#23544)""
  Revert "Build: Upgrade min gradle to 3.3 (elastic#23544)"
  Build: Upgrade min gradle to 3.3 (elastic#23544)
  Fix took assertion in response filter test
  Search took time should use a relative clock
  Adds toString() to snapshot operations in progress
  Docs: fix a typo in transport client's put-mapping.asciidoc (elastic#23607)
  Use include-tagged macro for high level client docs (elastic#23438)
  Update fill-column in .dir-locals.el to 100 characters
  Setup keystore during integration tests (elastic#22966)
  Fix typo 'Elastisearch' -> 'Elasticsearch' (elastic#23633)
  Comment and blank line cleanups (elastic#23647)
  docs: guidelines for students and teachers (elastic#23648)
  Fix MapperService StackOverflowError (elastic#23605)
rjernst pushed a commit that referenced this pull request Apr 12, 2017
This commit creates a keystore and adds settings to it during the
cluster formation for integration tests. Users can define a
`keyStoreSetting` in build files for settings that need to be placed in
the keystore.
@rjernst rjernst added the v5.4.0 label Apr 12, 2017
@Tim-Brooks Tim-Brooks deleted the keystore_work branch November 14, 2018 14:48
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure >non-issue Team:Delivery Meta label for Delivery team v5.4.0 v6.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants