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

Bug/jenkins 37425 run stop toast changes #515

Merged
merged 127 commits into from Sep 27, 2016

Conversation

cliffmeyers
Copy link
Contributor

@cliffmeyers cliffmeyers commented Sep 21, 2016

Description

  • See JENKINS-37425.
  • This PR updates the Favorite cards to use the same exactly component as the rest of dashboard for start, stop and replay of builds.

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.
  • Ran Acceptance Test Harness against PR changes.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
    • Test the Run, Stop and Replay buttons in the Favorite cards.
    • Ensure the "OPEN" link in the Toast still works.
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@jenkinsci/code-reviewers @reviewbybees

Cliff Meyers added 30 commits August 25, 2016 21:14
…L to the corresponding UI URL, to avoid encoding bugs
…s; add some assets for testing and in prep of "Run Pipeline" refactor
…iple "libraries" (e.g. JDL and CoreJS) in the src/main/webapp/assets directory
… be shared across plugins; support multiple subscriptions; still needs testing
…d to be caused by two SSE connections being made using the same clientId
…plicate listener for each subscription added
…prevents them from being added; in place until we find a more elegant fix in upstream code
…o it can toggle or show only run or stop; update Activity tab to use new RunButton at top for non multi-branch
Cliff Meyers added 8 commits September 22, 2016 11:14
# Conflicts:
#	blueocean-core-js/package.json
#	blueocean-core-js/src/js/components/ReplayButton.jsx
#	blueocean-core-js/src/js/components/RunButton.jsx
#	blueocean-dashboard/package.json
#	blueocean-personalization/package.json
#	blueocean-web/package.json
…w handled directly by RunButton / ReplayButton; delint
…od; fix a bug on Run Details where replaying multiple times would not work because the button was not having its state reset when bound to a new Run
…ardCards down into PipelineCard - where it's more sensible to be; update all shrinkwraps
# Conflicts:
#	blueocean-dashboard/npm-shrinkwrap.json
#	blueocean-personalization/npm-shrinkwrap.json
#	blueocean-web/npm-shrinkwrap.json
@@ -7,34 +7,14 @@ import { connect } from 'react-redux';
import { createSelector } from 'reselect';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a lot of logic from this component down into PipelineCard. Instead of this component picking out the data and passing a ton of props down into the card, it made sense to just pass the object down and let PipelineCard do that work.

text: `Stopping "${name}" #${runId}...`,
});
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Run/ReplayButton components do all the work of run, stop and replay... so this code can go.

>
{favoriteCards}
</TransitionGroup>
</div>
</FavoritesProvider>
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that this component is just rendering a stack of cards the render method is a lot simpler.

Fetch.fetch(stopPipelineUrl, fetchOptions);
};
},

updateRun(jobRun) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All this is handled by Run/ReplayButton now.


.svg-icon-inner {
fill: #CCC;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding some hover states for the start / stop / replay icons.

background-image: url('./icons/stop-button-blue-dark.svg');
}
}
/* asset has different padding than material-icon asset used in .run-button: nudge it slightly */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Stop button now uses a SVG inline to again avoid the need for 4 different external SVG assets. The SVG is tiny, so it shouldn't be bad at all.

toastService.newToast({
text: `Queued "${name}"`,
});
runApi.startRun(this.props.runnable)
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 the most significant change above: since we create "Started" and "Stopping" toasts immediately when the REST API call returns, we don't need to listen to SSE to create these toasts. The old code was a lot more complex than it needed to be.

});
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes a bug where the button got stuck in replaying state if going through the start -> stop -> rerun cycle more than once.


const shortNameMatches = flattenedCapabilities.filter(longName => shortNames.indexOf(longName) !== -1);
if (shortNameMatches.length > 0) {
return true;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Easy for someone to pass an array as the first argument rather than spreading it, so this logic handles either case (and has associated tests).

@ghost
Copy link

ghost commented Sep 26, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@cliffmeyers
Copy link
Contributor Author

Passed ATH locally. 2016-09-26 15:33:10,427 [Thread-3] INFO (InputStreamHandler.java:47) com.github.eirslett.maven.plugins.frontend.lib.DefaultGulpRunner - OK. 292 total assertions passed. (5m 10s)
`

autoNavigate
/>
</IfCapability>
<ReplayButton
Copy link
Member

Choose a reason for hiding this comment

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

I did a quick chance of anon read only access - seems ok (from what I can tell)

@michaelneale
Copy link
Member

michaelneale commented Sep 27, 2016

That's a lot of code to review - so so not sure if my read of it counts, but it seems sensible (lots of removed messy code, which seems nice) and seems to work.

I assume you ran the latest ATH - after doing a clean -DcleanNode? (I also kicked off: https://ci.blueocean.io/job/ATH-Jenkinsfile/job/master/6/ on CI - just to be sure).

This gets a 🐝 from me, but I would like @sophistifunk or @scherler to take a look if they are around if possible.

(also using -beta dependency that needs to change, may need updating to master as a few other changes went in, yada yada YMMV)

@sophistifunk
Copy link
Collaborator

Code looks OK to me 🐝

@cliffmeyers
Copy link
Contributor Author

cliffmeyers commented Sep 27, 2016

ATH passed locally. 2016-09-27 12:02:35,585 [Thread-3] INFO (InputStreamHandler.java:47) com.github.eirslett.maven.plugins.frontend.lib.DefaultGulpRunner - OK. 316 total assertions passed. (4m 3s)

@cliffmeyers cliffmeyers merged commit ca8ec53 into master Sep 27, 2016
@cliffmeyers cliffmeyers deleted the bug/JENKINS-37425-run-stop-toast-changes branch September 27, 2016 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants