-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Support 1 Kibana and 1 Elasticsearch URL as input params #9760
Conversation
@LeeDr is this still WIP? Can we close it out? |
💔 Build Failed |
Oops. This slipped out of my radar. Yes, this would still be good to get in as we are driving to run kibana tests on Cloud. I'll rebase and get this going again. |
💔 Build Failed |
jenkins test this |
💚 Build Succeeded |
src/test_utils/es/es_test_config.js
Outdated
port: testEsPort, | ||
auth: testEsUsername + ':' + testEsPassword, | ||
username: testEsUsername, | ||
password: testEsPassword, |
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.
Is there any risk in doing:
getUrlParts() {
let protocol;
let hostname;
let port;
let auth;
let username;
let password;
if (process.env.TEST_ES_URL) {
// Allow setting one complete TEST_ES_URL for Es like
// https://elastic:changeme@myCloudInstance:9200
const testEsUrl = url.parse(process.env.TEST_ES_URL);
// have to remove the ":" off protocol
protocol = testEsUrl.protocol.slice(0, -1);
hostname = testEsUrl.hostname;
port = parseInt(testEsUrl.port, 10);
username = testEsUrl.username;
password = testEsUrl.password;
} else {
// Allow setting any individual component(s) of the URL,
// or use default values (username and password from shield.js)
protocol = process.env.TEST_ES_PROTOCOL || 'http';
hostname = process.env.TEST_ES_HOSTNAME || 'localhost';
port = parseInt(process.env.TEST_ES_PORT, 10) || 9220;
username = process.env.TEST_ES_USERNAME || admin.username;
password = process.env.TEST_ES_PASSWORD || admin.password;
}
console.log(protocol);
return {
protocol,
hostname,
port,
auth: `${username}:${password}`,
username,
password,
};
}
(edited: removed too much first time)
test/kibana_test_server_url_parts.js
Outdated
port: testKibanaPort, | ||
username: testKibanaUsername, | ||
password: testKibanaPassword, | ||
auth: `${testKibanaUsername}:${testKibanaPassword}` |
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.
import { kibanaUser } from './shield';
import url from 'url';
let protocol;
let hostname;
let port;
let username;
let password;
let auth;
// Allow setting one complete TEST_KIBANA_URL for Kibana like
// https://elastic:changeme@myCloudInstance:5601
if (process.env.TEST_KIBANA_URL) {
const testKibanaUrl = url.parse(process.env.TEST_KIBANA_URL);
// have to remove the ":" off protocol
protocol = testKibanaUrl.protocol.slice(0, -1);
hostname = testKibanaUrl.hostname;
port = parseInt(testKibanaUrl.port, 10);
username = testKibanaUrl.username;
password = testKibanaUrl.password;
} else {
// Allow setting any individual component(s) of the URL,
// or use default values (username and password from shield.js)
protocol = process.env.TEST_KIBANA_PROTOCOL || 'http';
hostname = process.env.TEST_KIBANA_HOSTNAME || 'localhost';
port = parseInt(process.env.TEST_KIBANA_PORT, 10) || 5620;
username = process.env.TEST_KIBANA_USERNAME || kibanaUser.username;
password = process.env.TEST_KIBANA_PASSWORD || kibanaUser.password;
}
export const kibanaTestServerUrlParts = {
protocol,
hostname,
port,
auth: `${username}:${password}`,
username,
password,
};
maybe make things more consistent with the ES file, too.
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.
Thanks for the suggestion! I made that change and it is cleaner.
src/test_utils/es/es_test_config.js
Outdated
testEsUsername = process.env.TEST_ES_USERNAME || admin.username; | ||
testEsPassword = process.env.TEST_ES_PASSWORD || admin.password; | ||
} | ||
console.log(testEsProtocol); |
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.
remove these console.log
s
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.
LGTM, with some minor suggestions.
💚 Build Succeeded |
💚 Build Succeeded |
src/test_utils/es/es_test_config.js
Outdated
let password; | ||
|
||
// Allow setting one complete TEST_ES_URL for Es like https://elastic:changeme@myCloudInstance:9200 | ||
if (process.env.TEST_ES_URL) { |
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.
Can we just return an object here if TEST_ES_URL is defined? Then we can get rid of all the variables and last return.
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.
getUrlParts() {
// allow setting one complete TEST_ES_URL for ES like https://elastic:[email protected]:9200
if (process.env.TEST_ES_URL) {
const testEsUrl = url.parse(process.env.TEST_ES_URL);
return {
...testEsUrl,
port: parseInt(testEsUrl.port, 10),
auth: `${testEsUrl.username}:${testEsUrl.password}`,
testEsUrl.protocol.slice(0, -1),
};
}
const username = process.env.TEST_ES_USERNAME || admin.username;
const password = process.env.TEST_ES_PASSWORD || admin.password;
return {
protocol: process.env.TEST_ES_PROTOCOL || 'http',
hostname: process.env.TEST_ES_HOSTNAME || 'localhost',
port: parseInt(process.env.TEST_ES_PORT, 10) || 9220,
auth: `${username}:${password}`,
username,
password,
};
}
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.
That sounds pretty good. I'll try it that way.
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.
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.
For Kibana:
import { kibanaUser } from './shield';
import url from 'url';
function getUrlParts() {
// allow setting one complete TEST_KIBANA_URL for ES like https://elastic:[email protected]:9200
if (process.env.TEST_KIBANA_URL) {
const testKibanaUrl = url.parse(process.env.TEST_KIBANA_URL);
return {
...testKibanaUrl,
port: parseInt(testKibanaUrl.port, 10),
auth: `${testKibanaUrl.username}:${testKibanaUrl.password}`,
protocol: testKibanaUrl.protocol.slice(0, -1),
};
}
const username = process.env.TEST_KIBANA_USERNAME || kibanaUser.username;
const password = process.env.TEST_KIBANA_PASSWORD || kibanaUser.password;
return {
protocol: process.env.TEST_KIBANA_PROTOCOL || 'http',
hostname: process.env.TEST_KIBANA_HOSTNAME || 'localhost',
port: parseInt(process.env.TEST_KIBANA_PORT, 10) || 9220,
auth: `${username}:${password}`,
username,
password,
};
}
export const kibanaTestServerUrlParts = getUrlParts();
src/test_utils/es/es_test_config.js
Outdated
} else { | ||
// Allow setting any individual component(s) of the URL, | ||
// or use default values (username and password from shield.js) | ||
protocol = process.env.TEST_ES_PROTOCOL || 'http'; |
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.
Do you still need/use the other TEST_ES_* environment variables? Can we get rid of them if we are now providing the full URL?
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.
I didn't want to break backward compatibility if anybody was just overriding a port or something like that. I don't know if anybody is using any of them, but they were documented.
src/test_utils/es/es_test_config.js
Outdated
let password; | ||
|
||
// Allow setting one complete TEST_ES_URL for Es like https://elastic:changeme@myCloudInstance:9200 | ||
if (process.env.TEST_ES_URL) { |
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.
For Kibana:
import { kibanaUser } from './shield';
import url from 'url';
function getUrlParts() {
// allow setting one complete TEST_KIBANA_URL for ES like https://elastic:[email protected]:9200
if (process.env.TEST_KIBANA_URL) {
const testKibanaUrl = url.parse(process.env.TEST_KIBANA_URL);
return {
...testKibanaUrl,
port: parseInt(testKibanaUrl.port, 10),
auth: `${testKibanaUrl.username}:${testKibanaUrl.password}`,
protocol: testKibanaUrl.protocol.slice(0, -1),
};
}
const username = process.env.TEST_KIBANA_USERNAME || kibanaUser.username;
const password = process.env.TEST_KIBANA_PASSWORD || kibanaUser.password;
return {
protocol: process.env.TEST_KIBANA_PROTOCOL || 'http',
hostname: process.env.TEST_KIBANA_HOSTNAME || 'localhost',
port: parseInt(process.env.TEST_KIBANA_PORT, 10) || 9220,
auth: `${username}:${password}`,
username,
password,
};
}
export const kibanaTestServerUrlParts = getUrlParts();
💔 Build Failed |
jenkins test this |
💔 Build Failed |
💔 Build Failed |
I've never seen this failure before where it logged
|
jenkins test this |
💚 Build Succeeded |
@tylersmalley would you please check the latest commits and see if you're good with it? |
@LeeDr did you want to update the ES part, or keep as is? |
@tylersmalley oops, yes, testing that change now... |
💔 Build Failed |
jenkins test this |
💔 Build Failed |
💔 Build Failed |
Unrelated timelion test failure which needs some fix;
|
jenkins test this |
💚 Build Succeeded |
@tylersmalley back to you. I made es_test_config.js similar to kibana_test_server_url_parts.js |
* Support 1 Kibana and 1 Elasticsearch URL as input params * Revert a previous change to test char substitution * Allow setting TEST_KIBANA_URL and TEST_ES_URL for Cloud testing * cleanup comment * Update docs * Refactor after PR review * Changes from review * fix default Kibana port to 5620 * Change es_test_config.js similar to kibana_test_server_url_parts.js
) * Support 1 Kibana and 1 Elasticsearch URL as input params * Revert a previous change to test char substitution * Allow setting TEST_KIBANA_URL and TEST_ES_URL for Cloud testing * cleanup comment * Update docs * Refactor after PR review * Changes from review * fix default Kibana port to 5620 * Change es_test_config.js similar to kibana_test_server_url_parts.js
This will allow running Kibana tests against and Elasticsearch and Kibana instance like;
And also still supports individual parameters like;