Added replica set card status messages #319

floreks · 2016-02-01T13:07:24Z

This is the current look of the card status. I've also done some refactoring during backend implementation.

I've had a problem with cluster started with gulp local-up-cluster. Events returned by it has empty Type property. No idea why. It works fine on my local cluster started from kubernetes repo fork.

Important: Need help in finding Pending icon. :)

@maciaszczykm @bryk PTAL

floreks · 2016-02-01T13:09:06Z

src/app/frontend/replicasetlist/replicasetcard.html

Whoever knows this icon name. Please tell me. :)

codecov-io · 2016-02-01T13:14:46Z

Current coverage is `79.06%`

Merging #319 into master will increase coverage by +0.08% as of 861bca5

@@            master    #319   diff @@
======================================
  Files           68      68       
  Stmts          552     554     +2
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit            436     438     +2
  Partial          0       0       
  Missed         116     116

Review entire Coverage Diff as of 861bca5

Powered by Codecov. Updated on successful CI builds.

bryk · 2016-02-01T15:40:52Z

Re icon: anything worth using on material icons? E.g., https://design.google.com/icons/#ic_timelapse ?
Can you look at others?

bryk · 2016-02-01T15:42:00Z

Or something from the notification list? https://design.google.com/icons/

bryk · 2016-02-01T15:45:49Z

Re performance: It seems that for every card we query for events. I think it was supposed to query for events only when not all pods are running. Am I correct?

Sample log:

2016/02/01 16:44:25 Incoming HTTP/1.1 GET /api/replicasets request from [::1]:48812
2016/02/01 16:44:25 Getting list of all replica sets in the cluster
2016/02/01 16:44:25 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Getting list of pods error events
2016/02/01 16:44:26 Outcoming response to [::1]:48812 with 201 status code

bryk · 2016-02-01T15:52:14Z

Should this be an error? It is not when I test it

bryk · 2016-02-01T15:53:44Z

src/app/backend/events.go

Hey, now instead of one api call we have N api calls. Please fix :)

There are N api calls even now. I've only refactored it and extracted function.

https://github.com/kubernetes/dashboard/blob/master/src/app/backend/events.go#L149

Oh, yes. Sorry. Can you add a todo so that we know what to fix?

Sure. This PR is quite big so i will do that in next PR.

floreks · 2016-02-01T16:30:04Z

Should this be an error? It is not when I test it

Check my initial comment :)

I've had a problem with cluster started with gulp local-up-cluster. Events returned by it has empty Type property. No idea why. It works fine on my local cluster started from kubernetes repo fork.

You can check if they are empty on replica set detail events tab. If there will be no warning icons then it means that event types are empty and no errors can be recognized.

floreks · 2016-02-01T16:31:32Z

Re performance: It seems that for every card we query for events. I think it was supposed to query for events only when not all pods are running. Am I correct?

This may be misleading, because log info is on the start of function and then in the function itself is check whether pod is in some fail state in order to get events.

floreks · 2016-02-02T08:48:18Z

Re icon: anything worth using on material icons? E.g., https://design.google.com/icons/#ic_timelapse ?
Can you look at others?

I couldn't find anything better than timelapse, so I've used it.

floreks · 2016-02-02T08:49:31Z

src/app/backend/replicasetcommon.go

Should it be public?

No need IMO

floreks · 2016-02-02T08:57:13Z

I've changed some comments and renamed functions. PTAL

bryk · 2016-02-02T09:12:34Z

src/app/backend/replicasetcommon.go

Hmm.... I'd reverse this statement. The reason is that if new status is added, the function will be broken. If you list running and success here, then everything would be fine.

Makes sense.

bryk · 2016-02-02T09:29:41Z

The icons need some kind of tooltip that explain what they mean. I.e., the timelapse icon should have description that says that some pods are pending.

bryk · 2016-02-02T09:37:01Z

Should this be an error? It is not when I test it

Check my initial comment :)

Hmm.. I'm running on a "real" cluster and I still see this as pending. We should fix this, IMO. How about some kind of heuristics? I.e., when there is no event type, then check whether reason has Failed substring. This should cover most cases, as I'm looking at various failure reasons. What do you think?

floreks · 2016-02-02T10:48:32Z

I've extracted new functions to new file. It feels more related to events than to replicasetcommon. I need to adjust some comments only, but you can take a look.

bryk · 2016-02-02T11:16:43Z

src/app/backend/eventscommon.go

Hmm... This is equivalent to checking for only first event's type. How about for every event you check whether it has type or not. This is the way we should do this, because, I imagine, that some events have type and some not.

I think that if api will fill one event type then it will fill it for all events, but just to be extra safe I'll change.

bryk · 2016-02-02T11:18:45Z

Btw, I tested this on my instance. The tool we're building looks better and better! :)

bryk · 2016-02-02T11:23:26Z

We need some more tests on the backend and everything else looks good.

floreks · 2016-02-02T12:17:36Z

This PR has grown quite big :). PTAL

bryk · 2016-02-02T12:33:26Z

src/app/frontend/chrome/chrome.scss

I dont think this should be $secondary. It is a coincidence that our secondary color is red. It was pink not so long ago.

This should have color of {{warn-A700}} as defined, e.g., here: https://github.com/angular/material/blob/1f997951b5d480d04ea7d356107837e279d6a271/src/components/input/input-theme.scss#L70

bryk · 2016-02-02T12:38:37Z

Last two comments and LGTM.

bryk · 2016-02-02T12:38:54Z

Should we merge this after replica set renaming? I think yes.

floreks · 2016-02-02T12:44:43Z

Last two comments and LGTM.

Fixed

Should we merge this after replica set renaming? I think yes.

Unfortunately yes. Conflicts are going to kill me for sure :)

floreks · 2016-02-02T12:45:57Z

src/app/frontend/replicasetlist/replicasetcard.html

bryk · 2016-02-02T13:04:49Z

Ok, so LGTM on this. Let's wait for the rename to be done, and you'll rebase. Hopefully, without many problems :)

bryk · 2016-02-03T09:17:19Z

Rebase please :)

floreks · 2016-02-03T09:33:59Z

I really hope that I've merged it correctly :)

bryk · 2016-02-03T09:48:04Z

LGTM, nice job! Tested on prod, works like a charm. Merging.

Added replica set card status messages

googlebot added the cla: yes label Feb 1, 2016

floreks reviewed Feb 1, 2016
View reviewed changes

src/app/frontend/replicasetlist/replicasetcard.html Outdated

Copy link

Member Author

floreks Feb 1, 2016

Choose a reason for hiding this comment

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

Whoever knows this icon name. Please tell me. :)

floreks force-pushed the card-status branch 3 times, most recently from 76cb8ee to 7099b41 Compare February 1, 2016 14:21

bryk reviewed Feb 1, 2016
View reviewed changes

floreks force-pushed the card-status branch 3 times, most recently from fd4c4cb to 847c450 Compare February 2, 2016 08:46

floreks reviewed Feb 2, 2016
View reviewed changes

floreks force-pushed the card-status branch from 847c450 to 8a72ef0 Compare February 2, 2016 08:56

bryk reviewed Feb 2, 2016
View reviewed changes

floreks force-pushed the card-status branch from 8a72ef0 to c6eb30f Compare February 2, 2016 09:20

floreks force-pushed the card-status branch from c6eb30f to 950fd50 Compare February 2, 2016 10:46

bryk reviewed Feb 2, 2016
View reviewed changes

floreks force-pushed the card-status branch 4 times, most recently from 28911c9 to ffe7854 Compare February 2, 2016 12:09

floreks force-pushed the card-status branch from ffe7854 to f46bd87 Compare February 2, 2016 12:22

bryk reviewed Feb 2, 2016
View reviewed changes

floreks force-pushed the card-status branch from f46bd87 to 54b6d4f Compare February 2, 2016 12:41

floreks reviewed Feb 2, 2016
View reviewed changes

floreks force-pushed the card-status branch from 54b6d4f to fbe773f Compare February 2, 2016 13:09

floreks force-pushed the card-status branch 2 times, most recently from e179100 to 68c7c96 Compare February 3, 2016 09:33

Added replica set card status messages.

68c7c96

bryk added a commit that referenced this pull request Feb 3, 2016

Merge pull request #319 from floreks/card-status

c25dd5b

Added replica set card status messages

bryk merged commit c25dd5b into kubernetes:master Feb 3, 2016

floreks deleted the card-status branch February 3, 2016 09:49

floreks mentioned this pull request Feb 19, 2016

Replica set card status #302

Closed

Added replica set card status messages #319

Added replica set card status messages #319

Uh oh!

Conversation

floreks commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Feb 1, 2016

Current coverage is 79.06%

Uh oh!

bryk commented Feb 1, 2016

Uh oh!

bryk commented Feb 1, 2016

Uh oh!

bryk commented Feb 1, 2016

Uh oh!

bryk commented Feb 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

floreks commented Feb 1, 2016

Uh oh!

floreks commented Feb 1, 2016

Uh oh!

floreks commented Feb 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

floreks commented Feb 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryk commented Feb 2, 2016

Uh oh!

bryk commented Feb 2, 2016

Uh oh!

floreks commented Feb 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryk commented Feb 2, 2016

Uh oh!

bryk commented Feb 2, 2016

Uh oh!

floreks commented Feb 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryk commented Feb 2, 2016

Uh oh!

bryk commented Feb 2, 2016

Uh oh!

floreks commented Feb 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bryk commented Feb 2, 2016

Uh oh!

bryk commented Feb 3, 2016

Uh oh!

floreks commented Feb 3, 2016

Uh oh!

bryk commented Feb 3, 2016

Uh oh!

Uh oh!

Current coverage is `79.06%`