Skip to content
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

Events Tab implementation #81

Merged
merged 9 commits into from
Mar 27, 2020
Merged

Conversation

johnpoth
Copy link
Collaborator

This is a first stab at #36 to support events.

Hopefully we'll be able to reuse most of this for other resource types (via inheritance or composition).

We could also display a list of resource names similar to what we do for namespaces to support things like custom resources.

Once we agree on the display I'll polish things up a bit :)

HTH !

Note: I had to modify lib/ui/blessed/element.js for performance reasons.

Copy link
Owner

@astefanutti astefanutti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! That is very cool!

I think we can reuse the approach of requesting the table for the pods as well. It's less data and the only way I know to get pod ages without timing sync issue with the client.

lib/client.js Outdated Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
lib/ui/blessed/element.js Show resolved Hide resolved
.get({ generator: update_events_table.bind({ events_table, debug, highlightEvent })})
cancellations.add(key, cancellation);
return promise;
}).catch(error => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think error should be caught in the fail method of the spinner otherwise it keeps spinning.

top : '50%',
bottom : '1',
width : '100%',
height : '50%-1',
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems the status bar at the bottom is not displayed.

lib/ui/events.js Outdated Show resolved Hide resolved
lib/ui/events.js Show resolved Hide resolved
lib/ui/events.js Outdated Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
@astefanutti
Copy link
Owner

I've just re-tested it and got the following issue:

Screenshot 2020-03-02 at 18 32 00

I open the event tab. Then the events display correctly. I trigger an event. like scaling a deployment, then the new events display with undefined.

@johnpoth
Copy link
Collaborator Author

I've tried to reproduce scaling deployments up and down without any luck. I'm wondering if this is specific to kamel-k that creates events but doesn't populate some standard fields (like namespace).

Out of curiosity, could you run describe on one of those events ? I'm pretty sure some fields are undefined which is what causes the rendering issues.

@johnpoth
Copy link
Collaborator Author

I think this raises an important case in that some fields aren't necessarily populated. As this will also be relevant in other Kubernetes Resources, I thought it would be okay to add a check in the listable and table blessed Prototypes. Added relevant commit. Thanks !

@astefanutti
Copy link
Owner

Awesome. Did you get a change to look at the status bar not displaying? Otherwise I think we can merge the PR and have a look afterwards.

@astefanutti
Copy link
Owner

Note to myself, check how sorting is implemented in:

$ kubectl get events --sort-by=.metadata.creationTimestamp

@astefanutti
Copy link
Owner

Let me resolve the conflicts and merge the PR!

@johnpoth
Copy link
Collaborator Author

For sure! I'm still working on handling <unknown> last seen values in some events camel-k sends. I can merge the work afterwards. Thanks !

@johnpoth
Copy link
Collaborator Author

@astefanutti resolved merge conflicts, I also pushed hanlding <unknown> timestamps that should resolve the issue regarding the status bar you raised ? It needs some polishing but I think it's worth the effort especially if we are to reuse the timestamped columns for other resources. I'll try using the isValid function from moment.js that should clean things up a bit. Thanks !

@astefanutti
Copy link
Owner

@johnpoth Thanks a lot! Let's merge the PR and iterate. Once it's polished enough, I'll cut a release with all the good stuff that has been done since 0.7.0.

@astefanutti astefanutti merged commit 7017f02 into astefanutti:master Mar 27, 2020
@johnpoth johnpoth deleted the events branch March 30, 2020 08:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants