Skip to content

Conversation

@vikram-raj
Copy link
Member

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/dev-console Related to dev-console labels Sep 16, 2019
@andrewballantyne
Copy link
Contributor

@vikram-raj I think we probably want to drop that grey circle under the Overview header.

I think we probably want to explain why it doesn't have an overview, so maybe something like:

image

Will need UX for the verbiage (cc @serenamarie125 )

I am leaning towards thinking we should keep the title so there isn't just a void in "why don't I see the visualization on this pipeline?" kind of questions. Thoughts Serena?

@vikram-raj
Copy link
Member Author

@andrewballantyne Agree, and there is a separate issue for that and we need confirmation from the issue. Once UX will confirm it then I will remove that.

Copy link
Contributor

@andrewballantyne andrewballantyne Sep 16, 2019

Choose a reason for hiding this comment

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

pipeline.spec nor pipeline.spec.tasks are guaranteed by the type. Recommend handling the possible NPEs as well.

Also, could we get an explicit type on this variable - like type the FC?

Copy link
Contributor

Choose a reason for hiding this comment

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

In frontend/public/module/k8s/index.ts, this type is used for other components that this component calls.

export type K8sResourceKind = {
  apiVersion?: string;
  kind?: string;
  metadata?: ObjectMetadata;
  spec?: {
    selector?: Selector;
    [key: string]: any
  };
  status?: {[key: string]: any};
  type?: {[key: string]: any};
  data?: {[key: string]: any};
};

Copy link
Member Author

Choose a reason for hiding this comment

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

@andrewballantyne
Copy link
Contributor

Well, since the design stuff is tracked in another ticket - I'll just review the problem at hand... and that was a NPE breaking things. You fixed that, so I guess that's pretty much it for this PR.

/lgtm
/test e2e-aws-console

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2019
@vikram-raj
Copy link
Member Author

@andrewballantyne Yeah, we get the input from the UX then I will fix the design issue as well.

@vikram-raj
Copy link
Member Author

/assign @christianvogt

@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, vikram-raj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2019
@spadgett spadgett added this to the v4.2 milestone Sep 17, 2019
@openshift-merge-robot openshift-merge-robot merged commit f250854 into openshift:master Sep 17, 2019
@vikram-raj vikram-raj deleted the fix-odc-1858 branch September 17, 2019 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants