#23605: Add docker image ref to downloading image task event #23714
Conversation
|
Can someone point me out where I should add additional tests for those Task Events? |
…sk event" This reverts commit 9e64860.
…as in deployments view fo job, display details dynamically in each of rows of deployment history as table with name and detail columns
|
Hey @tgross it took me some time, TL;DR; Should I keep working on this, or should I change the way how this will be represented? I did not want to implement on my own any custom user interface, so I did check how tab for deployment of job looks like, and tried to recreate similar experience from there. Happy to change it to whatever you think needs an improvement. This PR would still require adding e2e tests and providing fixtures. Also I found that in many places, annotations of task event, so details as they are being returned from API and how the field is named in frontend - have duplicate of information, sometimes displayMessage is also put into Annotations/Details. Should I refactor task events and somehow remove those annotations somehow or rather the message should be refactored to shorter version, so more information can be found by expanding the details? Also I was thinking that possibly it would be better if we can move deployment history as a separate section in job? 
Also I found another view of Recent Events for a Task, should we align it there too? (Show details button added etc.) 
The outcome is as in screenshots attached. 
|
|
Thanks for this @tskorupka! I'm going to tag-in @philrenaud on reviewing this, as he's our web UI expert and he'll definitely have more useful thoughts on the UI changes than I will. 😁 |
|
Hi @tskorupka, and thanks for this contribution. My advice here would be to try to get details to emerge:
As such, my suggestion is to append the docker image name, if present in details, to the This has the added benefit of allowing the "Search Events" bar to just work with the image name, too, and is upstream enough that the task pages and task log flyouts will have the image name included in them as well. |
ui/app/models/task-event.js
Outdated
| @@ -17,11 +20,16 @@ export default class TaskEvent extends Fragment { | |||
| @attr('date') time; | |||
| @attr('number') timeNanos; | |||
| @attr('string') displayMessage; | |||
| @attr({ defaultValue: () => ({}) }) details; | |||
|
|
|||
| get message() { | |||
| let message = simplifyTimeMessage(this.displayMessage); | |||
| return message; | |||
There was a problem hiding this comment.
Consider appending the image name via conditional check here
|
Hey @philrenaud, thanks for such comprehensive answer. I agree with your suggestions, but I also think upfront and about possible future requests of having such details being shown, that is why I was thinking about having a generic way of displaying that kind of information. There could be a lot of other/different information being stored in annotations/details, that users could look for, and we could leverage that functionality to refactor task event messages to show less content and maybe restrict it to specific amount of characters, so it would not be flooded with information there. Adding specific methods to cover edge cases where the annotation/detail should be displayed will only introduce chaos to the code, that is something that I wanted to skip, and it could grow bigger. If searching is one of the cases where this edge case should be covered by appending the detail into display message field, I would rather suggest extending search method with lookup by detail. Just let me know if this is meaningful, if not I will rewrite the code by the requested by you changes. Kind Regards, |
|
I appreciate the feedback @tskorupka and the future-proofing concern here is valid, but IMO it's not enough to warrant arbitrary consideration of task event detail properties without them being added explicitly. The UI has a an implicitly high signal-to-noise ratio that we try to maintain by being selective about what sort of data we add in — not because of something like bandwidth over the wire, but because users have communicated to us that their use of the UI is often aided by things being pretty opinionated in where/how they should look for things. As far as "adding chaos to the code", I understand the concern as well, but image names are ubiquitous enough and specific enough that I suspect they're in a field of their own in terms of surfaceable properties. I am comfortable with the idea of a model-attribute-level check like this. |
…erience as in deployments view fo job, display details dynamically in each of rows of deployment history as table with name and detail columns" This reverts commit 9ef8664.
|
Hey @philrenaud, done. |
|
Change looks great, thanks! There's an acceptance test for searching deployment history and I'd consider adding a test there. We use Mirage for mocking data in our acceptance tests, and the one for task-events is at https://github.com/hashicorp/nomad/blob/859087640a74fd074f4a86ba2ddef71e46ae4017/ui/mirage/factories/task-event.js#L21-L22 This looks like it might be a little tricky and esoteric to Ember/Mirage, so please let me know if you'd like help with this test. Finally, we have a convention of adding changelog entries for contributions; would you please run Thank you so much for all of this, it's a really nice change! |
|
Hey @philrenaud, Thanks, great news.
Adding assertion and extending existing test is fine?
Will do. Kind Regards, |
Yep, that's exactly what I'd do here. |
Improves on #23605