Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

Use kibana server hostname for puppeteer to access pages #99

Merged
merged 3 commits into from
Sep 29, 2020

Conversation

zhongnansu
Copy link
Member

Issue #, if available:
Currently the url for puppeteer to access is a complete url passed from UI, including the schema, hostname and port. However this brings some issues when deploying on a cloud service such as AWS ELB, which has settings for security group. Since puppeteer is only accessing the url from localhost, it doesn't even need to go the ELB first and route back to itself. Instead it can access http://localhost:5601:/app/dashboard#/<dashboard_id> directly.

Description of changes:

  • This is a temporary fix to replace url prefix with a hardcoded LOCAL_HOST = "http://localhost:5601" constant. However since the hostname and port number are configured in kibana.yml file, which the new Kibana plugin platform doesn't provide access now. This issue is documented Access kibana.yml from plugin to read server.host, server.port and server.basePath #98 and tracked once there is a proper solution.
  • remove unused dependencies, move some packages from dependency to devDependency

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@@ -66,3 +66,5 @@ export enum TRIGGER_TYPE {
}
// https://www.elastic.co/guide/en/elasticsearch/reference/6.8/search-request-from-size.html
export const DEFAULT_MAX_SIZE = 10000;

export const LOCAL_HOST = 'http://localhost:5601';
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be hard-coding the host like this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally, we should extract the server host/port from kibana.yml, but the new platform applies some restrictions to access certain fields. More details can be found in this issue #98

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get the local host from a window.location or document method instead? I think those should be fine to access in the new platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

That window.location won't work on server side. The UI side still keeps the usage of window.location which is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack, miss from me- forgot this was on server-side

@@ -1,4 +1,3 @@
import { async } from 'rxjs/internal/scheduler/async';
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the changes in this file related to the title of the PR? I feel this is a different optimization refactor

Copy link
Member Author

Choose a reason for hiding this comment

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

That's just an unused import

Copy link
Contributor

Choose a reason for hiding this comment

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

Not just this line, I'm referring to the other changes in the file as well- meant to highlight the whole file

Copy link
Member Author

Choose a reason for hiding this comment

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

The other change is just rewriting the import to typescript style.

Copy link
Contributor

@davidcui1225 davidcui1225 left a comment

Choose a reason for hiding this comment

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

LGTM- just some small comments and questions to address

@zhongnansu zhongnansu merged commit bcc8fab into opendistro-for-elasticsearch:dev Sep 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants