-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[Monitoring] Support for unlinked deployments #28278
Merged
chrisronline
merged 34 commits into
elastic:master
from
chrisronline:monitoring/unlinked_deployment
Jan 25, 2019
Merged
Changes from 20 commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
9efe952
Unlinked deployment working for beats
chrisronline 3534720
Use better constant
chrisronline f5b3086
Merge remote-tracking branch 'elastic/master' into monitoring/unlinke…
chrisronline fa77948
Show N/A for license
chrisronline 5fda7e1
Rename to Unlinked Cluster
chrisronline 02108f2
Use callout to mention unlinked cluster
chrisronline 5fb7138
PR feedback
chrisronline 4f3a64d
Use fragment
chrisronline 7abe26c
Speed up the query by using terminate_after
chrisronline 61dff35
Handle failures more defensively
chrisronline d5e2263
Merge remote-tracking branch 'elastic/master' into monitoring/unlinke…
chrisronline 3590d8a
Remove unnecessary msearch
chrisronline dbc878b
PR feedback
chrisronline 220bfe3
PR feedback and a bit of light refactor
chrisronline 694921c
Updated text
chrisronline 136d23a
Merge remote-tracking branch 'elastic/master' into monitoring/unlinke…
chrisronline 4b168ed
Merge remote-tracking branch 'elastic/master' into monitoring/unlinke…
chrisronline 83ac0bf
Merge remote-tracking branch 'elastic/master' into monitoring/unlinke…
chrisronline d5fa37f
Add api integration tests
chrisronline f2ee66c
Localize call out
chrisronline cc99fe8
Update loc pattern
chrisronline 0e2e760
Fix improper i18n.translate usage
chrisronline 2d6cf9e
Merge remote-tracking branch 'elastic/master' into monitoring/unlinke…
chrisronline f35eed3
Revert "Fix improper i18n.translate usage"
chrisronline 0849281
Revert "Update loc pattern"
chrisronline 9a87135
Ensure the unlinked deployment cluster counts as a valid cluster
chrisronline cbe32b7
Sometimes, you miss the smallest things
chrisronline c3f1dfc
Ensure the unlinked cluster is supported, in that users can click the…
chrisronline 8aba3cd
Update tests
chrisronline 753d9d8
PR feedback. Simplifying the flag supported code and adding more tests
chrisronline 5c426cd
Update naming
chrisronline 12d7337
Rename to Standalone Cluster
chrisronline 1e1d1ca
Remove unnecessary file
chrisronline d9cc7df
Move logic for setting isSupported to exclusively in flag supported c…
chrisronline File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 changes: 7 additions & 0 deletions
7
x-pack/plugins/monitoring/public/components/cluster/listing/index.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
export { Listing } from './listing'; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 changes: 1 addition & 6 deletions
7
x-pack/plugins/monitoring/public/views/cluster/listing/index.html
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,3 @@ | ||
<monitoring-main name="listing"> | ||
<monitoring-cluster-listing | ||
pagination-settings="clusters.pagination" | ||
sorting="clusters.sorting" | ||
on-table-change="clusters.onTableChange" | ||
clusters="clusters.data" | ||
></monitoring-cluster-listing> | ||
<div id="monitoringClusterListingApp"></div> | ||
</monitoring-main> |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 logic appears to be appropriately reused in a bunch of places. Is the thought here that used a predefined constant is better than
undefined
because it ends up in the global scope, and we don't want to add logic to the global scope handling?(I'm asking to better understand your reasoning rather than "this is not right!")
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.
Yup basically. I didn't want to use an empty string for
cluster_uuid
and I felt something likeundefined
ornull
misrepresented what's going on.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.
But looking at the name,
cluster_uuid
, I feel like that's exactly what it means though?I do think one problem with using
undefined
/null
in the short term is that we redirect to the cluster listing page if multiple clusters are detected and it's not supplied / found. I wouldn't want you to change that behavior in this PR, but I do think we should stop redirecting automatically from the cluster listing page, which would then open up using it for that purpose.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 have any concerns with using this constant as the identifier for this? While I see your side of things, I feel like
null
orundefined
, while technically true for this, make it seem like some sort of error scenario/state where it's not really for the user. I feel pretty good about this name, though we should change it toUnlinked Cluster
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 don't see any problem with it.
cluster_uuid
is a fixed length identifier in ES, so you shouldn't have any unlikely collision opportunity. The constant's name is great, although I agree with the rename until "Deployment" is more widely applied.