Add service images widget #2487

jpellizzari · 2017-05-01T17:27:45Z

Implements #2478, depends on https://github.com/weaveworks/service-ui/pull/495

Displays information about the status of images for a given kubernetes service. This is an initial proof-of-concept to use a reference for doing more complex integrations between services.

The link is clickable and will navigate to Flux using the service-ui router that is passed to it via context.

Introduces a <CloudFeature /> component that changes the context from Scope to service-ui, allowing for components to access any part of the state tree.

jpellizzari · 2017-05-01T17:31:17Z

@fbarl Please review and let me know if you have any questions on how it works. The best way to test is to use PROXY_SCOPE_COMPONENT=true in the service-ui repo with this branch checked out.

rade · 2017-05-01T17:41:42Z

The link is clickable

Where exactly in the flux ui does it go? AFAIK the flux ui only has link destinations for an entire service, not for service + image.

jpellizzari · 2017-05-01T18:44:42Z

@rade It takes you to the /flux/:orgId/services/:serviceName. The hover effect triggers when the entire list body is moused-over, to indicate that you are clicking on a single entity. The list of container images is not individually clickable.

rade · 2017-05-01T18:51:45Z

The list of container images is not individually clickable.

Ah! But you have underlined "authfe" and "logging" items in your example. Do these go to the container detail panels? Or are they not clickable? (which, IMO, would be visually misleading, since every other underlined item is a link). Also, the blue "New image(s) available" text looks clickable since blue is a very commonly used for links. Like here in github :)

jpellizzari · 2017-05-01T19:09:56Z

But you have underlined "authfe" and "logging" items in your example. Do these go to the container detail panels?

I wrestled with the right way to represent this for a while. We indicate navigation with underlines, but clicking on either container image will take you to the same place in Flux. The compromise I came up with is to indicate that either is clickable, then on mouse-over indicate that they are indistinguishable.

The screenshot in the first comment is the :hover state of the component. The non-hover state is a white background.

I agree its an inconsistency. We could also go for what I call "the big dumb button" approach to indicate navigation:

Also, the blue "New image(s) available" text looks clickable since blue is a very commonly used for links. Like here in github :)

Those are clickable as part of the body of the list. The blue text is just to differentiate the text from the normal state.

rade · 2017-05-01T19:33:01Z

We could also go for what I call "the big dumb button"

That might be better - it avoids any confusion as to what is/isn't clickable, and what clicking will actually do. We could perhaps make it smaller, and place it on the same level as the heading (but right-aligned), so as to save precious vertical space and be less disruptive to the overall look and function of the details panel.

jpellizzari · 2017-05-01T23:32:26Z

Updated after feedback:

Opted for a link instead of a button, since we don't currently use a Button anywhere else.

I also changed the 'New Images available' text to green instead of blue.

rade · 2017-05-02T07:46:57Z

On which detail panels is this info displayed? I've looked at the code and can't work it out :(

Flux operates on k8s deployments, of which services can be seen as a specialisation. So the info should be shown in both.

rade · 2017-05-02T07:49:02Z

So the info should be shown in both.

We could even additionally show the info on pods and even containers, when these are part of a deployment.

fbarl

Really nice PR and it really paves the path for further integrations I now think will be smoother than I initially thought. :)

I left a few suggestions in the code and would add just one UI comment to them - the spinner seems to overflow the Node Details panel sometimes. You should be able to reproduce it if you keep the viewport height not to big and click around the nodes.

I think that the current View in Deploy link looks much better than the initial solution with underlined entries.

client/app/scripts/reducers/root.js

@@ -84,6 +84,7 @@ export const initialState = makeMap({
  viewport: makeMap(),
  websocketClosed: false,
  zoomCache: makeMap(),
+  serviceImages: makeMap({ fetching: false })


client/app/scripts/reducers/root.js

+        return state.setIn(['serviceImages', 'errors'], action.errors);
+      }
+
+      const service = find(action.services, s => s.ID === action.serviceId);


client/app/scripts/reducers/__tests__/root-test.js

@@ -678,4 +678,33 @@ describe('RootReducer', () => {
      constructEdgeId('def456', 'abc123')
    ]);
  });
+  it('receives images for a service', () => {


client/app/scripts/components/node-details/node-details-image-status.js

+
+  componentDidMount() {
+    if (this.props.serviceName) {
+      this.props.getImagesForService(this.props.params.orgId, this.props.serviceName);


client/app/scripts/components/node-details/node-details-image-status.js

+  }
+
+  handleServiceClick() {
+    this.props.router.push(`/flux/${this.props.params.orgId}/services/${encodeURIComponent(this.props.serviceName)}`);


client/app/scripts/components/node-details/node-details-image-status.js

+
+function getNewImages(images, currentId) {
+  // Assume that the current image is always in the list of all available images.
+  // Should be a safe assumption...


client/app/scripts/components/node-details/node-details-image-status.js

+        {fetching && <CircularProgress />}
+        {!fetching && errors && <p>Error: {JSON.stringify(map(errors, 'message'))}</p>}
+        {!errors && !fetching && !service && 'No service images found'}
+        {!errors && !fetching && service &&


client/app/scripts/components/node-details/node-details-image-status.js

+
+import { getImagesForService } from '../../actions/app-actions';
+
+const topologyWhitelist = ['services'];


fbarl · 2017-05-02T13:56:12Z

On which detail panels is this info displayed? I've looked at the code and can't work it out :(

@rade, it should be displayed on the bottom of node details of each node in Services topology. The important thing is to test it in combination with https://github.com/weaveworks/service-ui/pull/495 which hasn't been merged yet. I did it by running SCOPE_PATH=~/Projects/src/github.com/weaveworks/scope SERVICE_HOST=frontend.dev.weave.works PROXY_SCOPE_COMPONENT=true yarn start from that service-ui branch locally (you'd probably want to change SCOPE_PATH though).

rade · 2017-05-02T14:04:17Z

it should be displayed on the bottom of node details of each node in Services topology

That's what I guessed. My point was that it should at least also be shown in the Deployment topology.

jpellizzari · 2017-05-02T18:38:40Z

@fbarl I updated this branch with your feedback, PTAL. I also added the image status widget to the 'deployments' view.

fbarl

I left one more tiny comment, otherwise LGTM.

client/app/scripts/components/node-details/node-details-image-status.js

+          return (
+            <div key={container.Name} className="wrapper">
+              <div className="node-details-table-node-label">{container.Name}</div>
+              <div className="node-details-table-node-label">{statusText}</div>


jpellizzari requested a review from fbarl May 1, 2017 17:31

jpellizzari self-assigned this May 1, 2017

jpellizzari force-pushed the 2478-flux-integration branch from 0cf02aa to 7dd308f Compare May 1, 2017 17:32

jpellizzari force-pushed the 2478-flux-integration branch from 7dd308f to 375fef8 Compare May 1, 2017 23:31

fbarl suggested changes May 2, 2017

View reviewed changes

fbarl approved these changes May 3, 2017

View reviewed changes

fbarl mentioned this pull request May 3, 2017

Move away from React.PropTypes #2496

Closed

jpellizzari force-pushed the 2478-flux-integration branch from 94a58b2 to cccfd50 Compare May 3, 2017 17:17

Added service images widget

3ec7cc8

jpellizzari force-pushed the 2478-flux-integration branch from cccfd50 to 3ec7cc8 Compare May 3, 2017 17:31

jpellizzari merged commit 54f7904 into master May 3, 2017

jpellizzari deleted the 2478-flux-integration branch May 3, 2017 17:57

fbarl mentioned this pull request May 4, 2017

Update node libraries #2292

Merged

rade mentioned this pull request Oct 5, 2017

scope/flux integration broken with new flux #2880

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add service images widget #2487

Add service images widget #2487

jpellizzari commented May 1, 2017 •

edited

Loading

jpellizzari commented May 1, 2017

rade commented May 1, 2017

jpellizzari commented May 1, 2017 •

edited

Loading

rade commented May 1, 2017

jpellizzari commented May 1, 2017 •

edited

Loading

rade commented May 1, 2017

jpellizzari commented May 1, 2017 •

edited

Loading

rade commented May 2, 2017

rade commented May 2, 2017

fbarl left a comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

fbarl commented May 2, 2017

rade commented May 2, 2017

jpellizzari commented May 2, 2017

fbarl left a comment

This comment was marked as abuse.


		import { getImagesForService } from '../../actions/app-actions';

		const topologyWhitelist = ['services'];

Add service images widget #2487

Add service images widget #2487

Conversation

jpellizzari commented May 1, 2017 • edited Loading

jpellizzari commented May 1, 2017

rade commented May 1, 2017

jpellizzari commented May 1, 2017 • edited Loading

rade commented May 1, 2017

jpellizzari commented May 1, 2017 • edited Loading

rade commented May 1, 2017

jpellizzari commented May 1, 2017 • edited Loading

rade commented May 2, 2017

rade commented May 2, 2017

fbarl left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

fbarl commented May 2, 2017

rade commented May 2, 2017

jpellizzari commented May 2, 2017

fbarl left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

jpellizzari commented May 1, 2017 •

edited

Loading

jpellizzari commented May 1, 2017 •

edited

Loading

jpellizzari commented May 1, 2017 •

edited

Loading

jpellizzari commented May 1, 2017 •

edited

Loading