-
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
[Logs Explorer] Rename test subjects and page objects #175711
[Logs Explorer] Rename test subjects and page objects #175711
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
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.
Data Discovery changes LGTM 👍
@elasticmachine merge upstream |
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.
Serverless test config change LGTM
// go to log explorer tab | ||
await testSubjects.click('logExplorerTab'); | ||
// go to logs explorer tab | ||
await testSubjects.click('logsExplorerTab'); | ||
await PageObjects.svlCommonNavigation.breadcrumbs.expectBreadcrumbExists({ | ||
deepLinkId: 'discover', |
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.
Should we also change deepLinkId
of observability Logs Explorer app? (there is an example of its use around line 175)
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.
Changed in 889e2a7
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.
Shouldn't we also rename the folder name?
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.
Renamed in 889e2a7
@@ -60,7 +60,7 @@ export function createTestConfig(options: CreateTestConfigOptions) { | |||
observability: { | |||
pathname: '/app/observability', | |||
}, | |||
observabilityLogExplorer: { | |||
observabilityLogsExplorer: { | |||
pathname: '/app/observability-log-explorer', |
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.
Shouldn't we rename this path as well?
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 think we should, I haven't changed it in this PR as I was mostly keen to get the test subj and telemetry updated, I'll mention it in the original issue so we don't miss it
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.
In fact, telemetry is also based on the URL maybe it makes more sense to do it here
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.
Change the app url in 889e2a7
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.
Infra changes: LGTM
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
Thanks for taking care of this!
|
||
This plugin is home to the `<LogExplorer />` component and related types. It implements several of the underlying concepts that the [Observability Log Explorer app](../observability_solution/observability_logs_explorer) builds upon. | ||
This plugin is home to the `<LogExplorer />` component and related types. It implements several of the underlying concepts that the [Observability Logs Explorer app](../observability_solution/observability_logs_explorer) builds upon. |
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.
nit: shall we rename also the component here and in the implementation to LogsExplorer
?
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.
Definitely, there are quite a few things that still have the singular name, I have started renaming these in batches to avoid major conflicts and long-running PRs. This PR is mostly for tests and the app url.
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
Just left a note about renaming the LogExplorer
component but the rest LGTM, thanks for this work!
- Renames test subjects and page objects - Renames test folders from `observability_log_explorer` to `observability_logs_explorer` - Changes app url from `observability-log-explorer` to `observability-logs-explorer` and adds another app for redirects Related to elastic#171991 --------- Co-authored-by: Kibana Machine <[email protected]>
- Renames test subjects and page objects - Renames test folders from `observability_log_explorer` to `observability_logs_explorer` - Changes app url from `observability-log-explorer` to `observability-logs-explorer` and adds another app for redirects Related to elastic#171991 --------- Co-authored-by: Kibana Machine <[email protected]>
observability_log_explorer
toobservability_logs_explorer
observability-log-explorer
toobservability-logs-explorer
and adds another app for redirectsRelated to #171991