Redesigned Overview #1335
Redesigned Overview #1335
Conversation
spadgett
force-pushed
the
overview
branch
3 times, most recently
from
8fe857b to
d3bf28b
Compare
| }); | ||
| AlertMessageService.clearAlerts(); | ||
|
|
||
| var getBreakpoint = function() { |
There was a problem hiding this comment.
Feels like this function could move into a filter or service for us to use elsewhere, guessing this won't be the last time we need to do it
| } | ||
| }, 50); | ||
|
|
||
| $(window).on('resize', onResize); |
There was a problem hiding this comment.
use a scoped event, 'overview.resize'
| }]; | ||
| overview.filterBy = 'name'; | ||
|
|
||
| overview.viewByOptions = [{ |
There was a problem hiding this comment.
i'm debating whether these should be under renderOptions
overview.renderOptions = {
...
filterBy
filterByOptions
viewByOptions
...
}
I know historically renderOptions were only things we names consistently across pages. But these seem like things that probably should be named consistently across pages :)
There was a problem hiding this comment.
It's funny, I was debating removing renderOptions since some directives that don't have isolated scope look for it. For instance,
The settings here apply only to the overview, but renderOptions implies that they might be used outside of this controller. We have some copied code in other controllers as well that sets renderOptions when not needed.
As an aside, we might remove that check from project-filter. I don't think it's needed anymore since it was removed from the nav a while back?
There was a problem hiding this comment.
Good point, we may not even need renderOptions any more
| }; | ||
|
|
||
| // Group a collection of resources by app label. Returns a map where the key | ||
| // is the app label value and the value is an array of object, sorted by |
| // is the app label value and the value is an array of object, sorted by | ||
| // `metadata.name`. | ||
| var groupByApp = function(collection) { | ||
| return AppsService.groupByApp(collection, 'metadata.name'); |
There was a problem hiding this comment.
Can this be more generic and put it in a LabelsService instead? then you pass it which label key you want to group by. Or was the thinking that if we kept it as group by app but changed how the grouping worked that it would just be under this same service.
There was a problem hiding this comment.
Or was the thinking that if we kept it as group by app but changed how the grouping worked that it would just be under this same service.
Yeah, that was the thought, although I'm OK moving it to LabelsService as well.
There was a problem hiding this comment.
Ok thats fine, I think I'm ok with it either way.
| }; | ||
|
|
||
| // Track view-by state in localStorage. | ||
| var viewByKey = $routeParams.project + '/view-by'; |
There was a problem hiding this comment.
make this key specific to overview in case we add a view-by somewhere else later
| } | ||
|
|
||
| var isPod = function(apiObject) { | ||
| return apiObject && apiObject.kind === 'Pod'; |
There was a problem hiding this comment.
not necessarily better but could also do
_.isMatch(apiObject, { kind: 'Pod' });
| var visibleReplicationControllers = getVisibleReplicationControllers(deploymentConfig); | ||
| _.each(visibleReplicationControllers, function(replicationController) { | ||
| var rcNotifications = getNotifications(replicationController); | ||
| _.assign(notifications, rcNotifications); |
There was a problem hiding this comment.
if multiple visible RCs have the same notifications cant these clash? do the keys for the RC notifications that come back have unique keys for the object?
There was a problem hiding this comment.
The alert ID for RC notifications contains the object UID. (And for pods, we have the grouping logic.)
| var rcByDC = {}; | ||
| _.each(overview.replicationControllers, function(replicationController) { | ||
| var dcName = getDeploymentConfigName(replicationController) || ''; | ||
| if (!dcName || !overview.deploymentConfigs[dcName]) { |
There was a problem hiding this comment.
does this mean if i delete a DC and it doesn't cascade to delete the RCs that I'm going to see a bunch of old RCs showing up that aren't scaled up?
Thanks @jwforres. I added a checklist for feedback and follow-on work to the description, and I have local changes for most of your comments to this point. Let me know if you'd like me to push. |
|
@jwforres Noticed I'm missing some |
|
@jwforres Should we reorder the builds and pipelines menu items in the nav to match the order in the overview?
|
|
I would hesitate to switch nav order on people if we can avoid it. Muscle memory is a thing :) |
|
|
||
| // Sort the visible replication controllers. | ||
| _.each(rcByDC, function(replicationControllers, dcName) { | ||
| var ordered = orderObjectsByDate(replicationControllers, true); |
There was a problem hiding this comment.
I remember on builds we hit an issue when we got back two builds with the same timestamp, same in theory is possible on triggered RCs. Should we have a secondary sort that we can pass to orderObjectsByDate, in this case secondary sort would be the deployment number descending. I'd also be ok opening a follow up issue for this one since I think its a general problem.
| } | ||
|
|
||
| var deployment = overview.deployments[deploymentName]; | ||
| var visibleRelicaSets = _.filter(replicaSets, function(replicaSet) { |
There was a problem hiding this comment.
typo visibleReplicaSets, here and below
| }); | ||
| var ordered = DeploymentsService.sortByRevision(visibleRelicaSets); | ||
| overview.replicaSetsByDeployment[deploymentName] = ordered; | ||
| // TODO: Need to check if this really works for failed / canceled rollouts. |
There was a problem hiding this comment.
was this an existing TODO or do you still need to resolve this?
| var visibleRelicaSets = _.filter(replicaSets, function(replicaSet) { | ||
| return isReplicaSetVisible(replicaSet, deployment); | ||
| }); | ||
| var ordered = DeploymentsService.sortByRevision(visibleRelicaSets); |
There was a problem hiding this comment.
so you are sorting by revision here but ordering by date on RCs for a DC, can't we just do this same thing for DCs then to fix the potential issue with colliding timestamps
| // TODO: Need to check if this really works for failed / canceled rollouts. | ||
| // It might need to be reworked. | ||
| overview.currentByDeployment[deploymentName] = _.head(ordered); | ||
| // var deploymentRevision = DeploymentsService.getRevision(deployment); |
There was a problem hiding this comment.
is this commented out code part of checking the TODO above?
| _.set(overview, ['deploymentConfigsByPipeline', bcName], dcNames); | ||
| _.each(dcNames, function(dcName) { | ||
| state.pipelinesForDeploymentConfig[dcName] = state.pipelinesForDeploymentConfig[dcName] || []; | ||
| state.pipelinesForDeploymentConfig[dcName].push(buildConfig); |
There was a problem hiding this comment.
should this be sorted
It's not displayed directly, although recentPipelinesByDeploymentConfig is.
| } | ||
|
|
||
| // Reset these maps. | ||
| overview.recentPipelinesByBuildConfig = {}; |
There was a problem hiding this comment.
its a little confusing from a maintenance perspective that some of these maps are on "state" and some of them are directly on overview. I get that state only needs to have the things that have to be passed down into the list row, just seems easy to typo down the road when making changes.
There was a problem hiding this comment.
I'm hesitant to put things in state that aren't needed by list row. Then it's not obvious when you change something here if it impacts the list row component.
| $scope.project = project; | ||
| state.context = context; | ||
|
|
||
| var updateReferencedImageStreams = function() { |
There was a problem hiding this comment.
should we pull this up out of the watches since all the other functions are defined above?
There was a problem hiding this comment.
It needs access to context returned from ProjectsService.get :/
There was a problem hiding this comment.
Ok probably something we should refactor later then, it doesn't really need the context, i think it really just needs to know the project
| overview.horizontalPodAutoscalers = hpaData.by("metadata.name"); | ||
| groupHPAs(); | ||
| Logger.log("autoscalers (subscribe)", overview.horizontalPodAutoscalers); | ||
| }, {poll: limitWatches, pollInterval: 60 * 1000})); |
There was a problem hiding this comment.
any reason you didn't switch these next few to use DEFAULT_POLL_INTERVAL
There was a problem hiding this comment.
No, that was an oversight.
| state.limitRanges = response.by("metadata.name"); | ||
| }); | ||
|
|
||
| watches.push(DataService.watch("imagestreams", context, function(imageStreamData) { |
There was a problem hiding this comment.
thinking we should have all the watches first, then the limited watches, then the always polled, then the lists, and then the gets. I think most of them were in that order besides this one.
|
@jwforres I've updated the build sorting with a |
|
@jwforres I believe I've addressed all comments to this point |
| }); | ||
|
|
||
| if (buffer.length > numLines) { | ||
| buffer = _.takeRight(buffer, numLines); |
There was a problem hiding this comment.
if we get a flood of messages this could generate a lot of unnecessary array operations, when we only care about the last chunk. Although if we skip over showing anything then you won't get the scroll through effect of seeing it logged out and disappearing, so i'm not sure there is a better alternative.
There was a problem hiding this comment.
Yeah, not sure what's best. At least in this case, the buffer is max 8 lines, so we aren't ever keeping too much data in memory. And we're only calling _.takeRight on a small array.
| notification.byType = _.mapValues(byType, function(alerts) { | ||
| return _.map(alerts, function(alert) { | ||
| return _.escape(alert.message); | ||
| }).join('<br>'); |
There was a problem hiding this comment.
does it need a double <br> to get whitespace between the messages? might make the popup too big in some cases i guess
| if (!_.isEmpty(row.services)) { | ||
| _.set(row, 'selectedTab.networking', true); | ||
| } else { | ||
| _.set(row, 'selectedTab.networking', true); |
There was a problem hiding this comment.
this if and else are resulting in the same thing, looks like the first one is supposed to be different
| }, { alerts: row.state.alerts }); | ||
| }; | ||
|
|
||
| // TODO: Pulled from dc.js, but we should probably make the dialog generic and reuse for the deployment config page. |
There was a problem hiding this comment.
agreed
Added a follow on item to use the same dialog for the deployment config page.
| if (replicationController.metadata.uid !== row.current.metadata.uid) { | ||
| row.state.alerts["cancel-deployment"] = { | ||
| type: "error", | ||
| message: "Deployment " + rcName + " no longer latest." |
There was a problem hiding this comment.
do you want rcName here or the version number? also this isnt a complete sentence, "is no longer latest" would be better, maybe "is no longer the latest"
| return 1; | ||
| } | ||
|
|
||
| return right.weight - left.weight; |
There was a problem hiding this comment.
if weights are equal fallback to name
app/scripts/services/deployments.js
Outdated
| } | ||
|
|
||
| leftVersion = parseInt(leftVersion, 10); | ||
| rightVersion = parseInt(rightVersion, 10); |
There was a problem hiding this comment.
shouldn't we handle bad version annotations that parse as NaN? would expect these to be treated like non-existing versions
There was a problem hiding this comment.
Yeah, they should parse as NaN. The second parameter is radix.
There was a problem hiding this comment.
Right but if they parse as NaN don't we want to fall back to sorting by name?
There was a problem hiding this comment.
Right but if they parse as NaN don't we want to fall back to sorting by name?
Fixed
| var hideAlert = function(alertID, namespace) { | ||
| var key = alertHiddenKey(alertID, namespace); | ||
| localStorage.setItem(key, 'true'); | ||
| }; |
There was a problem hiding this comment.
isnt this duplicating these that we already had put into AlertMessageService, which is actually referenced in the quota warnings below.
There was a problem hiding this comment.
Yeah, this code was moved from serviceGroupNotifications, which probably was written before the helpers were added to AlertMessageService. I'll update it to use the AlertMessageService methods.
|
Addressed latest comments. |
| .text-muted(); | ||
| text-transform: uppercase; | ||
| .sublabel { | ||
| font-size: (@component-label - 1); |
There was a problem hiding this comment.
not seeing where @component-label is defined, is that defined in px? does the variable subtraction do the right thing on px sizes?
There was a problem hiding this comment.
app/styles/_variables.less
44:@component-label: 11px;
does the variable subtraction do the right thing on px sizes?
Yes, that works.
app/styles/_overview.less
Outdated
| } | ||
| .section-title { | ||
| .h4(); | ||
| &:not(.no-border) { |
There was a problem hiding this comment.
do we need to do a negation here, or can we add the border and then set it to none if .no-border is set? not selectors are a little more expensive
app/views/overview/_builds.html
Outdated
| <div class="col-sm-7"> | ||
| <div ng-if="!(overviewBuilds.recentBuildsByBuildConfig[buildConfig.metadata.name] | hashSize)"> | ||
| No builds. | ||
| <!-- <a href="" ng-if="('buildconfigs/instantiate' | canI : 'create')" ng-click="startBuild(buildConfig)" class="small mar-left-sm">Start Build</a> --> |
| @@ -0,0 +1,15 @@ | |||
| <div class="route-service-bar-chart"> | |||
| <h5>Traffic Split</h5> | |||
| <div ng-repeat="backend in routeServices.backends"> | |||
There was a problem hiding this comment.
worth doing a trackBy uid here?
There was a problem hiding this comment.
No UID available, we just have name. We could do a track by backend.name, but I think it's possible for the same name to be there twice (even if it doesn't make sense), which would cause a runtime error.
| <a ng-href="{{row.apiObject | editYamlURL}}">Edit YAML</a> | ||
| </li> | ||
| <li ng-if="(pod = row.firstPod(row.current)) && ('pods/log' | canI : 'get')" role="menuitem"> | ||
| <a ng-href="{{pod | navigateResourceURL}}?tab=logs">View Logs</a> |
There was a problem hiding this comment.
This really makes me want to have a logs tab on deployments / statefulsets that just adds a Pod switcher in addition to the container switcher. Follow-on though
There was a problem hiding this comment.
This really makes me want to have a logs tab on deployments / statefulsets that just adds a Pod switcher in addition to the container switcher. Follow-on though
+1
| <div ng-if="row.apiObject.kind === 'DeploymentConfig'"> | ||
| <div ng-if="pipeline = row.pipelines[0]"> | ||
| <p> | ||
| This deployment config is part of pipeline |
| pods="row.getPods(row.current)" | ||
| containers="row.current.spec.template.spec.containers" | ||
| profile="compact" | ||
| alerts="row.sate.alerts" |
| pods="row.getPods(row.current)" | ||
| containers="row.current.spec.template.spec.containers" | ||
| profile="compact" | ||
| alerts="row.sate.alerts" |
app/views/overview/_networking.html
Outdated
| <h3> | ||
| <a ng-href="{{service | navigateResourceURL}}">{{service.metadata.name}}</a> | ||
| </h3> | ||
| <!-- <span class="text-muted mar-right-xs"> --> |
| <i class="fa fa-external-link small" aria-hidden="true"></i> | ||
| </span> | ||
| <span ng-if="!(route | isWebRoute)">{{route | routeLabel}}</span> | ||
| <route-warnings route="route" service="service"></route-warnings> |
There was a problem hiding this comment.
wondering if route warnings should be taking into account alternate backends, i know we didnt already just thinking as something for later
There was a problem hiding this comment.
Yeah I had previously opened an issue, #1272
|
Added the route beside the app name:
|
|
[merge] |
|
\o/ |
|
Evaluated for origin web console merge up to d7f1197 |
|
Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1161/) (Base Commit: 136be55) |
https://trello.com/c/QxbqJsoK
Updated overview using the Patternfly list view with rows that expand to reveal more detail.
Contains code from @rhamilto. Based on designs from @ajacobs21e @lizsurette and @ncameronbritt
Fixes #236
Fixes #619
Fixes #658
Fixes #729
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1389458
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1401134
TODO
getBreakpointto a serviceview-bylocal storage key specific to overviewcanIchecks for kebab actionsrenderOptionsrow.state.breakpointrather thanhidden-*classes to show and hide the metrics summaryDEFAULT_POLL_INTERVALvisibleReplicaSetstypoLabelSelector.matchesrecentPipelinesByBuildConfigin the viewAlertMessageService.permanentlyHideAlertinResourceAlertsServiceFollow-on Work
pipeline.alpha.openshift.io/uses