Add edit dashboards command #1573
Conversation
|
Pinging @elastic/fleet (Team:Fleet) |
|
Thanks @jsoriano for the feedback! I can certainly add documentation in this PR. |
|
Hey @jsoriano 👋 I was thinking about the output of this command and realised it wouldn't be a good user experience if the user tried to make multiple dashboards editable and one of them failed: the command would stop on the first failure and fail to report any success or other potential failures. For example, if dashboard ids 123,456,789 were passed and the queries worked for 123 but not for 456 and 789, then the user would only see an error message for 456, despite 123 being successfully updated, and they wouldn't know that 789 would fail too. In this spirit, I've made some changes while incorporating your suggestions. The output currently looks like so: With 2 valid ids and 1 invalid id: And if there was an issue with building the dashboard URLs, I think the output should still be successful: What do you think? |
|
/test |
| kibanaURL, err := url.Parse(kibanaHost) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to retrieve Kibana URL: %w", err) | ||
| } |
There was a problem hiding this comment.
I am getting this output:
The following dashboards are now editable in Kibana:
https://127.0.0.1:5601#/view/elastic_agent-a148dc70-6b3c-11ed-98de-67bdecd21824
https://127.0.0.1:5601#/view/elastic_agent-1badd650-d136-11ed-b85f-4be0157fc90c
I think we are missing to add the /app/dashboards part here.
| } | |
| } | |
| dashboardsURL, err := kibanaURL.JoinPath("app", "dashboards") | |
| if err != nil { | |
| return "", fmt.Errorf("failed to build base dashboards URL: %w", err) | |
| } |
There was a problem hiding this comment.
Yep, sorry, just realised that too, pushed a fix.
| urls, err := dashboardURLs(*kibanaClient, updatedDashboardIDs) | ||
| if err != nil { | ||
| cmd.Println(fmt.Sprintf("\nFailed to retrieve dashboard URLS: %s", err.Error())) | ||
| cmd.Println(fmt.Sprintf("The following dashboards are now editable in Kibana:\n%s", strings.Join(updatedDashboardIDs, "\n"))) |
There was a problem hiding this comment.
This will probably never happen, because we were using the URL before. But good to handle the error just in case 👍
|
@jsoriano I iterated based on your feedback. With the command returning an error when at least one dashboard could not be updated, I thought printing the failures was redundant with the final command output. This is what it looks like in the current implementation: Success: Failure: WDYT? Note that the case where the dashboard URLs doesn't return a failure if all dashboards were successfully edited, e.g.: But, as you pointed out, this should in theory never happen 🙂 |
| errMsgs := make([]string, 0, len(failedDashboardUpdates)) | ||
| for _, value := range failedDashboardUpdates { | ||
| errMsgs = append(errMsgs, value) | ||
| } |
There was a problem hiding this comment.
I wonder if errors.Join() could help here https://pkg.go.dev/errors#Join. I haven't used it a lot, it was recently added to the standard library.
|
|
||
| Error: failed to make one or more dashboards editable: failed to export dashboard 456: could not export saved objects; API status code = 400; response body = {"statusCode":400,"error":"Bad Request","message":"Error fetching objects to export","attributes":{"objects":[{"id":"456","type":"dashboard","error":{"statusCode":404,"error":"Not Found","message":"Saved object [dashboard/456] not found"}}]}} | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Maybe we have to remember that after modifying the dashboard, it should be exported.
There was a problem hiding this comment.
Do you mean that this error message could be confusing to the user?
There was a problem hiding this comment.
Ah no, the error is fine, probably I put my comment in the wrong place.
I mean that after following this howto the user is going to be able to edit a dashboard. Maybe we have to mention in some place that after editing the dashboard, the developer should remember to export it with elastic-package export.
|
@jsoriano I pushed a change making use of Was there any action you were thinking of regarding this comment?
|
It would be only to mention that after modifying the dashboard, the developer needs to remember to export it, but it is fine as it is now too. |
Ah, I see! OK, I can add e.g. to the output and the doc for extra clarity. |
|
Latest version 🙂 |
💚 Build Succeeded
History
|
Summary
This PR adds the
elastic-package edit dashboards <dashboardIds>command using the method implemented in #1565.Relates to elastic/kibana#170517
Example usage
Using the interactive dashboard selection prompt
Passing a comma-separated list of dashboard ids (success)
Output:
Passing a comma-separated list of dashboard ids (partial failure)
In this example the first two ids are valid and the third one is not):
Output: