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

Recreate build pipeline within tectonic console #75

Conversation

benjaminapetersen
Copy link
Contributor

w/@sg00dwin, bah, wasn't going to squash your commit into mine, sorry about that. 😦

Looking over a few more details from the old pipeline's flow, then I'll remove the WIP. Any feedback welcome.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 29, 2018
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2018
@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 29, 2018
@benjaminapetersen
Copy link
Contributor Author

Pending:
screen shot 2018-05-29 at 10 49 54 am

Build in progess:
screen shot 2018-05-29 at 10 51 24 am

A complete flow:
screen shot 2018-05-29 at 10 52 55 am

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 29, 2018
@spadgett
Copy link
Member

/cc @spadgett @rhamilto

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Thanks @benjaminapetersen. I realize this is still WIP, but wanted to give you some feedback. I have not looked closely at the CSS

$pipeline-in-progress-color: #0088ce;
$pipeline-new-color: #bee1f4;
$pipeline-pending-color: #d1d1d1;
$pipeline-success-color: #3f9c35;
Copy link
Member

Choose a reason for hiding this comment

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

can we use $color-pf-* for colors in the palette?

https://www.patternfly.org/styles/color-palette/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sg00dwin I'm fine with this if you are.

Copy link
Member

Choose a reason for hiding this comment

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

We can. I gave @benjaminapetersen the hex incorrectly.

@benjaminapetersen can you replace with the following vars.

$pipeline-aborted-color: $color-pf-black-300;
$pipeline-border-color: $color-pf-black-300;
$pipeline-failed-color: $color-pf-red-100;
$pipeline-in-progress-color: $color-pf-blue-400;
$pipeline-new-color: $color-pf-blue-100;
$pipeline-pending-color: $color-pf-black-300;
$pipeline-success-color: $color-pf-green-400;
$pipeline-require-attention-color: $color-pf-gold-400;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, will update.

import { resourcePath } from './utils';
import { fromNow } from './utils/datetime';

const getBuildNumber = (resource: any) => _.get(resource, ['metadata', 'annotations', 'openshift.io/build.number']);
Copy link
Member

Choose a reason for hiding this comment

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

You can leave : any off and it's the same (here and below). I'd rather not specify that everywhere when it's not needed.

But you might use : K8sResourceKind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect, I'd rather use K8sResourceKind.

const getStages = (status: any) => status && status.stages || [];
const getJenkinsStatus = (resource: any) => {
const status = _.get(resource, ['metadata', 'annotations', 'openshift.io/jenkins-status-json']);
return status && JSON.parse(status);
Copy link
Member

Choose a reason for hiding this comment

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

Need to handle exceptions if it's not valid JSON.

import { fromNow } from './utils/datetime';

const getBuildNumber = (resource: any) => _.get(resource, ['metadata', 'annotations', 'openshift.io/build.number']);
const getStages = (status: any) => status && status.stages || [];
Copy link
Member

Choose a reason for hiding this comment

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

const getStages = status => (status && status.stages) || [];

Copy link
Member

Choose a reason for hiding this comment

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

I guess this is actually the same due to order of precedence, but I think the parens make it more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup but I can update.



const BuildSummaryStatusIcon: React.SFC<BuildSummaryStatusIconProps> = ({status}) => {
let statusClass = _.lowerCase(status);
Copy link
Member

Choose a reason for hiding this comment

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

We should use const instead of let when possible (here and below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also polishing up for next push.

</div>
}

const BuildAnimation = ({status}) => {
Copy link
Member

Choose a reason for hiding this comment

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

I'd use this shorthand

const BuildAnimation = ({status}) => <div>...</div>;

Should this have a type?

let buildUrl = getJenkinsBuildURL(resource);
if(pending) {
return <div className="build-pipeline__stage-actions text-muted">
<a href={buildUrl}>Input Required</a>
Copy link
Member

Choose a reason for hiding this comment

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

target="_blank" rel="noopener noreferrer"

return null;
}

const BuildStageTimestamp = ({timestamp}) => <div className="build-pipeline__stage-time text-muted">
Copy link
Member

Choose a reason for hiding this comment

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

types?

</div>;
};

// NOTE: is not instantiated, so need this wrapper?
Copy link
Member

Choose a reason for hiding this comment

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

I'd leave off the comment since it applies to all tsx files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, sorry didn't remove the [WIP] yet. Next push will eliminate the last of my notes-to-self 😄

@@ -38,6 +39,11 @@ export const BuildsDetails: React.SFC<BuildsDetailsProps> = ({obj: build}) => {
const duration = formatBuildDuration(build);

return <div className="co-m-pane__body">
<div className="row">
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's OK, but this is going to add some empty DOM nodes if it's not actually a build pipeline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To fix this I'd prob extract the helper functions from the top of the build-pipeline.tsx file into a module, perhaps components/utils/build-pipeline.tsx or module/build-pipeline.tsx, then import into both files. That would let me use one of the functions here to conditionally render the <row>.

Preferrable?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just check build.spec.strategy.type === 'JenkinsPipeline'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, def plenty reasonable. Lets go that route.

&:last-child:before {
display: none;
}
&.build-pipeline__stage--none {
Copy link
Member

Choose a reason for hiding this comment

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

Is the nesting necessary? BEM suggests you shouldn't nest...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sg00dwin I'll defer to you on the style

text-align: center;
}

@media (min-width: 480px) {
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to nest the media queries with the default rules so things are a bit more compartmentalized?

}

.build-pipeline__status-icon {
&--complete {
Copy link
Member

Choose a reason for hiding this comment

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

Here modifiers are nested, but elsewhere they are not. Seems like we should be consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

height: 100%;
padding: 0 ($pipeline-padding / 2);
}
.build-pipeline__stage {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: (here and throughout the file) alpha

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rhamilto is that standard in this repo? or have they followed another convention?
(asking cuz I'm still not a huge fan of alpha, ha.)

Copy link
Member

Choose a reason for hiding this comment

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

It is not (I'm not sure there is one), but it does appear to be in use within this partial.

Copy link
Contributor

@kans kans left a comment

Choose a reason for hiding this comment

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

  1. Please document how we can test this feature. We could potentially solve this problem in the future by adding different flavors of build configs to the YAML editor like we do for Roles.

  2. Along the same lines, I am concerned about the difficulty of writing a reliable integration test for the pipeline visualization (mostly async animations). What are your thoughts?

  3. In the not so distant past, we have seen trivially simple CSS animations use an entire CPU core in Chrome (a single blinking indicator). What does this do to performance?

  4. Porting features over to the new Console is a golden opportunity to re-evaluate them, whether that be scope, form, or implementation. This sort of CSS is practically impossible for a human to review and there is quite a bit of it. Lets make an intentional decision one way or the other: is this UI and its implementation what we want now, or something that is merely convenient to build?

justify-content: center;
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sg00dwin we could use @each for stuff like this next block, to reduce some repetition. I don't have a strong opinion, but here is a before / after:


// without a list
@media (min-width: $screen-md-min) {
  .build-pipeline__stage {
    width: (100% / 4);
  }
}

@media (min-width: $screen-lg-min) {
  .build-pipeline__stage {
    width: (100% / 5);
  }
}
@media (min-width: ($screen-lg-min + 200)) {
  .build-pipeline__stage {
    width: (100% / 6);
  }
}

@media (min-width: ($screen-lg-min + 400)) {
  .build-pipeline__stage {
    width: (100% / 7);
  }
}

// using a list
$pipelineStageScreenSize: (
  4 $screen-md-min,
  5 $screen-lg-min,
  6 ($screen-lg-min + 200),
  7 ($screen-lg-min + 400)
);

@each $screenSize in $pipelineStageScreenSize {
  @media (min-width: nth($screenSize, 2)) {
    .build-pipeline__stage {
      width: (100% / nth($screenSize, 1));
    }
  }    
}

Both will generate the same output css.

@benjaminapetersen
Copy link
Contributor Author

@kans yeah let me see if I can hit all of those:

  1. I'd love to see this for every PR, actually. A screenshot & "how do I use this?" is great for anyone reviewing, as well as historical documentation.

The Openshift QE V3 Test Files repo has quite a few bits of yaml and json to create resources in a variety of states. There are a handful of Pipelines options, for the sample below, I'm pulling the first one that came up:

# create a project
oc process -f https://raw.githubusercontent.com/openshift-qe/v3-testfiles/99d26e235b093431bff70e9edffc524559a9d664/pipeline/ui-pipeline-stage.yaml | oc create -f -

Then you should be able to navigate to a build page, see the pipeline, click action -> rebuild to run again, etc.

  1. Integration tests for animations... I'd probably expect an exceptionally good set of unit tests & integration tests before worrying about testing a css animation. Not sure there is a ton value in pursuing.

  2. Seems like an edge case, I'd prob trust a manual test for a css animation & worry about automated testing for all kinds of other things before worrying about a test case for this. Could be an enormous rabbit hole. Open to feedback, of course.

  3. Definitely open to re-evaluation. This is not the full pipeline experience from the old console. I'd say comment away on the items in the PR that are worth discussion and lets talk (its essentially 1 CSS file and 1 JS file). I think both @sg00dwin and I would be happy to kick around chagnes, optimizations, etc.

Thanks for the feedback!

@spadgett
Copy link
Member

In the not so distant past, we have seen trivially simple CSS animations use an entire CPU core in Chrome (a single blinking indicator). What does this do to performance?

@kans How have you profiled animations in the past?

I feel like, specifically for pipelines, they add a lot to the user experience. It's immediately apparent what stage is active and when the pipeline moves to the next stage.

@spadgett
Copy link
Member

cc @openshift/team-ux-review

@beanh66
Copy link
Member

beanh66 commented May 31, 2018

Thanks @spadgett, we will discuss in our design meeting today and will follow up this afternoon with comments.

@serenamarie125
Copy link
Contributor

@spadgett sorry bout the post 24 hour reply ... just connected with @sspeiche and others to get more background so we could provide a more thoughtful response.

The current design team was not involved with the pipeline design, although in our opinion we agree that it provides an positive user experience.

To provide a bit of history, we needed to have a CI/CD story for our platform...Jenkins is the #1 workflow/pipeline open source tool. The goal was to provide an integrated experience. The design "mirrored" the state in Build objects in OpenShift from Jenkins instead of forcing people to go back and forth. The original design process was a very long process driven by designers who did engage with customers during the design process. The design process was iterative.

The general feedback from customers has been that they really like it. There are some specific requests to improve on it, but these have yet to be addressed. Note that this is still a relatively new feature in the rate our customers roll out updates. So usage is picking up more now ...

One question going forward would be more around role based access. This feature would definitely be expected by an "app/dev" role, but unsure for admins.

@robszumski do you have additional thoughts?

@serenamarie125
Copy link
Contributor

One more note, OpenShift.io uses the same exact pipeline representation as OpenShift

@tlwu2013
Copy link
Contributor

tlwu2013 commented Jun 1, 2018

@beanh66, @cshinn, and I briefly chatted about the design yesterday in our sync.
We feel it seems fine to keep the current design to matchup the experience on App Console side.
So I think from design standpoint, this seems to be good go. But I can't comment on engineering/testing point of view, so I'll defer that to dev team for extended discussions.
And if this PR did introduce serious performance throttle, I am open to exploring tweaks to the design to see if we can improve that.

BTW, just FYI, here's the screen capture I did from the youtube provided by @beanh66 for showing the interaction OpenShift Console side:
https://drive.google.com/file/d/1NzXtv5sLSvbUruF4SNru-NmR8eYwThW7/view?usp=sharing

@benjaminapetersen benjaminapetersen force-pushed the build-pipelines-migration branch 2 times, most recently from 3872c8b to 36e8a78 Compare June 11, 2018 18:00
@spadgett
Copy link
Member

@benjaminapetersen @sg00dwin do you feel like this is ready for review?

@rhamilto can you review the styles?

@benjaminapetersen
Copy link
Contributor Author

Running it again locally today, but the scss doesn't seem to build correctly (i think its something local, not that there is a scss error).

@benjaminapetersen
Copy link
Contributor Author

benjaminapetersen commented Jun 11, 2018

Ok, local cache. solved.

Yeah, @spadgett @rhamilto I think its safe to give it a second round of review. @sg00dwin agreed?
@alecmerdler appreciate your thoughts on code style consistency or anything as well!

}
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Untested, but since we have that open topic regarding sass loops, An alternative could be to use a sass map, something like this:

// can also use a map, with key-value pairs
$pipeline-stages-per-row: (
  small: (
    column-no: 4,
    media-query-px: $screen-md-min
  )
  medium: (
    column-no: 4,
    media-query-px: $screen-lg-min
  )
  large: (
    column-no: 4,
    media-query-px: $screen-lg-min + 200
  )
  x-large: (
    column-no: 4,
    media-query-px: $screen-lg-min + 400
  )
)

// and use the map-get() function to get value by key...
@each $size, $map in $pipeline-stages-per-row {
  @media (min-width: map-get($map, media-query-px)) {
    .build-pipeline__stage {
      width: (100% / map-get($map, column-no));
    }
  }
}

map-get($map, the-key-to-get) is clearer than nth($screenSize, someIndex), but the code is a bit longer.

Copy link
Member

@rhamilto rhamilto left a comment

Choose a reason for hiding this comment

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

Overall is fine. It's not clear if the file is organized in some way. Sometimes things are alpha, sometimes not. Would be good to be consistent within the file.


.build-pipeline {
border: 1px solid $pipeline-border-color;
font-size: $pipeline-font-base;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: spacing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tab to space fixed.

@benjaminapetersen
Copy link
Contributor Author

benjaminapetersen commented Jun 12, 2018

@rhamilto @sg00dwin I'm good with choosing an organizational pattern going forward (and ideally even documenting it). Still not fond of how alphabetical breaks up logical groupings like height and width, or top,bottom,left,right, etc, but any organization is better than random.

@benjaminapetersen
Copy link
Contributor Author

@rhamilto opinion on the loops? Post refactor, prob worth assessing if its really an improvement or no.

height: 100%;
opacity: 0;
width: 100%;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just an observation, since we've been kicking around the topic. Most of the other existing SCSS files seem to hover around 50 lines or less. There are a couple in the range of 100.

350 lines is pretty big.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per the file size, I'm wondering if it would be good to break the elements down. I'm partial to the pattern I see in a lot of React apps where you have something like:

/public
  /components
    /build-pipeline
      |_ build-pipeline.js 
      |_ build-stage.js 
      |_ etc.

And do an

// build-pipeline.js
import './build-pipeline.scss';

// build-stage.js
import './build-stage.scss';

// etc.

But the current pattern is more or less one big file list in public/components so I don't necessarily want to go out of the norm.

Any thoughts? @rhamilto @sg00dwin
As well as @alecmerdler @spadgett since its a structure question.

Otherwise, I'm going to remove the [WIP] because we might be close to settled.

Copy link
Member

Choose a reason for hiding this comment

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

@benjaminapetersen Well, we can split it up to make it smaller, though a cursory look at the code, if you pull the rest out into a build-pipeline.scss partial, the majority of code would still be in build-stage.scss. Probably would still be 200 lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think our verbals on this indicated we don't want to split it up at this point.

@benjaminapetersen benjaminapetersen changed the title [WIP] Recreate build pipeline within tectonic console Recreate build pipeline within tectonic console Jun 12, 2018
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 12, 2018
return {};
}

const json = _.attempt(JSON.parse, status);
Copy link
Member

Choose a reason for hiding this comment

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

nit: It's no longer JSON now that you've parsed it... I'd swap these variable names.

const json = _.get(resource, ['metadata', 'annotations', 'openshift.io/jenkins-status-json']);
// ...
const status = _.attempt(JSON.parse, json);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

<span className={`build-pipeline__status-icon build-pipeline__status-icon--${statusClass}`}>
<span aria-hidden="true">
<span className={`fa ${icon} fa-fw`}></span>
</span>
Copy link
Member

Choose a reason for hiding this comment

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

Why the extra span? Couldn't this be...

<span className={`fa ${icon} fa-fw`} aria-hidden="true"></span>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure can, updating.

complete: 'fa-check-circle',
failed: 'fa-times-circle'
})[statusClass];
const hide = icon ? 'hide' : '';
Copy link
Member

Choose a reason for hiding this comment

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

Why use the hide class if we could just use JSX to hide the node. Is there some animation we want?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sg00dwin on the animation Q, otherwise I'll fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sg00dwin there isn't a .hide class in the build-pipelines.scss file, doing a grep -ri 'hide' public/components' doesn't reveal a generic hide class with an animation, so I'll update this to use jsx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be:

{!icon && <span className="build-pipeline__status-icon">
      <span className="fa fa-refresh fa-spin" aria-hidden="true"></span>
</span>}

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, suggest an sr-only span in there too for screen readers

return <React.Fragment>
<span className={`build-pipeline__status-icon build-pipeline__status-icon--${statusClass}`}>
<span aria-hidden="true">
<span className={`fa ${icon} fa-fw`}></span>
Copy link
Member

Choose a reason for hiding this comment

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

We check for empty icon above. Doesn't that mean this could result in class="fa fa-fw" with no icon here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably should be:

  return <React.Fragment>
    {icon && <span className={`build-pipeline__status-icon build-pipeline__status-icon--${statusClass}`}>
      <span className={`fa ${icon} fa-fw`} aria-hidden="true"></span>
    </span>}

    {!icon && <span className="build-pipeline__status-icon">
      <span className="fa fa-refresh fa-spin" aria-hidden="true"></span>
    </span>}
  </React.Fragment>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then the <React.Fragment> is not really needed, we will either do one or the other...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So probably should be:

  return icon
    ? <span className={`build-pipeline__status-icon build-pipeline__status-icon--${statusClass}`}>
      <span className={`fa ${icon} fa-fw`} aria-hidden="true"></span>
    </span>
    : <span className="build-pipeline__status-icon">
      <span className="fa fa-refresh fa-spin" aria-hidden="true"></span>
    </span>

};

const StagesNotStarted: React.SFC<StagesNotStartedProps> = ({stages}) => {
return !stages.length ? <div className="build-pipeline__stage build-pipeline__stage--none">No stages have started.
Copy link
Member

Choose a reason for hiding this comment

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

Prefer _.isEmpty

</div>;

const BuildStageName: React.SFC<BuildStageNameProps> = ({name}) => {
return <div title={name} className="build-pipeline__stage-name">
Copy link
Member

Choose a reason for hiding this comment

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

title is because long names get elided?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Think so:

.build-pipeline__stage-name {
  @include text-overflow();
  margin-bottom: $pipeline-padding + 3px;
}

Copy link
Member

Choose a reason for hiding this comment

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

that's correct

</div>;

const JenkinsInputUrl: React.SFC<JenkinsInputUrlProps> = ({obj, stage}) => {
const pending = stage && (stage.status === 'PAUSED_PENDING_INPUT');
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to check if stage is defined here. We don't check other places, so it will be an error before it gets to this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. hopefully the check in build.tsx means we won't start rendering any of this unless there are jenkins pipeline stages.

<BuildPipelineSummary obj={obj} />
<div className="build-pipeline__container">
<div className="build-pipeline__stages">
<StagesNotStarted stages={stages} />
Copy link
Member

Choose a reason for hiding this comment

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

This is a little confusing because it looks like StagesNotStarted is always visible, but then you loop over stages below. I think it's much clearer if you didn't use the component:

{_.isEmpty(stages)
  ? <div className="build-pipeline__stage build-pipeline__stage--none">No stages have started.</div>
  : stages.map(stage => <BuildStage obj={obj} stage={stage} key={stage.id} /> )}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can put the _.isEmpty() check up here, that makes sense.

@@ -38,6 +39,11 @@ export const BuildsDetails: React.SFC<BuildsDetailsProps> = ({obj: build}) => {
const duration = formatBuildDuration(build);

return <div className="co-m-pane__body">
<div className="row">
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just check build.spec.strategy.type === 'JenkinsPipeline'?

<div className="build-pipeline__container">
<div className="build-pipeline__stages">
<StagesNotStarted stages={stages} />
{ stages.map((stage => <BuildStage obj={obj} stage={stage} key={stage.id} /> )) }
Copy link
Member

Choose a reason for hiding this comment

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

Why the double parens (( )) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was just envious of Lisp the day I wrote this...

@benjaminapetersen
Copy link
Contributor Author

Addressing these & pushing an update shortly.

- Per task https://jira.coreos.com/browse/CONSOLE-453
- additions from @sg00dwin
  - Update scss to be smaller and less repetitive.
  - And change markup for when "no stages have started." state
  - Remove build details link since we're already show build details beneath pipeline
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 19, 2018

@benjaminapetersen: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/crud ee9a1d9 link /test crud
ci/prow/alm ee9a1d9 link /test alm
ci/prow/performance ee9a1d9 link /test performance

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@benjaminapetersen
Copy link
Contributor Author

Comments addressed, squashed & co-authored w/@sg00dwin.

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

/lgtm


const BuildLogLink: React.SFC<BuildLogLinkProps> = ({ obj }) => {
const link = getJenkinsLogURL(obj);
return link ? <div className="build-pipeline__link">
Copy link
Member

Choose a reason for hiding this comment

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

nit: does this work?

return link && <div> ... </div>;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I bet it would have! Woops.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2018
@stevekuznetsov
Copy link

/test backend
/test frontend

@openshift-merge-robot openshift-merge-robot merged commit 1b5b07b into openshift:master Jun 19, 2018
wtrocki added a commit to wtrocki/console that referenced this pull request Feb 10, 2021
TimothyAsirJeyasing pushed a commit to TimothyAsirJeyasing/console that referenced this pull request Aug 3, 2022
Migration: Moves Storage System List Page and Add Capacity Modal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet