#23605: Add docker image ref to downloading image task event

[tskorupka]

Improves on #23605

[tskorupka]

Can someone point me out where I should add additional tests for those Task Events? And actually an example how this assertion can be added?

[tskorupka]

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.

[tgross]

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. :grin:

[philrenaud]

Hi @tskorupka, and thanks for this contribution. My advice here would be to try to get details to emerge:

1. only if those details are important to the task event in the UI (docker image name makes sense to show, as requested in #23605, for example)
2. without adding extra interface if possible (it's already a small space here, and users don't want to click more than necessary)
3. at a more upstream location than the deployment-history component (as we surface these messages, as you've noticed, in other places throughout the UI as well)

As such, my suggestion is to append the docker image name, if present in details, to the task-event's message getter. You can see the pattern of using the simplifyTimeMessage() function to modify the message for the UI, and I suggest writing a method called appendImageName() or something similar there as well (You'd want to do a conditional check for this.details.Image or use an @alias etc.)

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.

[tskorupka]

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, Tomasz

[philrenaud]

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.

[tskorupka]

Hey @philrenaud, done.

[philrenaud]

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 make cl for this PR before final submission?

Thank you so much for all of this, it's a really nice change!

[tskorupka]

Hey @philrenaud,

Thanks, great news.

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

Adding assertion and extending existing test is fine?

Finally, we have a convention of adding changelog entries for contributions; would you please run make cl for this PR before final submission?

Will do.

Kind Regards, Tomasz

[philrenaud]

Adding assertion and extending existing test is fine?

Yep, that's exactly what I'd do here.

[tskorupka]

Still relevant, worthwhile adding?

[philrenaud]

Hi @tskorupka, I think still perfectly relevant. If you'd like, I can take a shot at adding the acceptance test assertion and changelog entry. Would you mind if I did?

[tskorupka]

Hey @philrenaud, it would be great if you do, sorry, I was offline for a while, I definitely look forward to adding this myself, but also as a collaborative effort I would be thankful ^^.

[shantanugadgil]

possibly related to https://github.com/hashicorp/nomad/issues/11024 and https://github.com/hashicorp/nomad/issues/11009