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
[FIXED JENKINS-41334] Adding parallel stages. #152
Conversation
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. |
@@ -80,35 +80,10 @@ public class ModelInterpreter implements Serializable { | |||
for (int i = 0; i < root.stages.getStages().size(); i++) { | |||
Stage thisStage = root.stages.getStages().get(i) | |||
try { | |||
// NOTE that this will eventually be moved into evaluateStage, once BO can | |||
// handle visualizing stage-inside-parallel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By "this" I assume you mean script.stage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup.
} else { | ||
result.append(','); | ||
if (branches.isEmpty() && parallelStages != null) { | ||
result.append("parallelStages {\n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not follow the RFC.
And revenge of the camelCase :) Why not go with parallel
as the RFC states? Confusion? There are plenty of synonyms or other contextually applickable things to parallel
to choose from if you don't want to use that word; threads
, simultaneous
, concurrent
. But I personally think that reusing the word stages
is best. It self explains in a way that it is sub stages to this stage although that could add to the meaning as it is not apparent they would run in parallel so it would change the implementation a bit since probably an attribute should then be added to declare them to be executed in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in that last case, why not allow all stages to potentially be run in parallel, even the outer ones :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compromise, basically. I believe that parallel
is too overloaded for sure, the synonyms for parallel
are completely new terms for Pipeline and so I'd prefer to avoid them (both to prevent the need for learning new terminology and, more importantly, to keep those terms free for possible later use!), and just stages
is too overloaded as well.
With this, stages
ends up meaning "execute these stages linearly" while parallel
means, as in Scripted, "execute these arbitrary blocks in parallel", and the new parallelStages
means, fairly explicitly, "execute these stages concurrently". While I don't have any further use for those distinctions now, I can envision some possible use cases in the future.
Utils.markStageSkippedForFailure(thisStage.name) | ||
} else if (skipUnstable(root.options)) { | ||
Utils.logToTaskListener("Stage '${thisStage.name}' skipped due to earlier stage(s) marking the build as unstable") | ||
Utils.markStageSkippedForUnstable(thisStage.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work for nested stages since there is no actual stage to mark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! Needs a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And done.
} | ||
|
||
if (firstError != null) { | ||
throw firstError |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit iffy whether nested stages will get their post run or not. Some tests should be added that checks for exceptions in various parts of a nested stage tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yyyup, this is buggy. Working on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the nested stages were doing post
fine, but the parent stage was not. So...juggling things around. Hooray.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parallelStages
seems reasonable to me. We seem to be shooting for a bit more verbose but clear names, like customWorkspace
.
But it is probably worth getting buy off from more people (and maybe updating the RFC?) and addressing questions like: can parallelStages
be used instead of stages
at the top of pipeline? Design-wise, I'd expect the answer to be yes.
The first thing someone is going to do when they see this:
pipeline {
stages {
stage("foo") {
paralllelStages {
stage("first") {
parallelStages {
stage("first-and-one") {
They'll want to do either these two things:
pipeline {
parallelStages { // this
stage("foo") {
agent { label 'someLabel' } // and this
stages { // and this
stage("first") {
parallelStages {
stage("first-and-one") {
And none of those three is supported in this implementation, right?
I'd almost rather push this out and do this in a way that is consistent with the stage
behavior elsewhere.
RFC already was updated. =) Lemme think on the top-level idea. Not sure about that. |
@abayer - making all |
@bitwiseman I think it will get more confusing trying to nest The alternative could be to make them both top-level only:
The big disadvantage I see with treating |
Oh, we're definitely not supporting multiple |
I agree that we should leave this inside a
|
There's a test that contains exactly that. ... Oh, I see, it's an error case. Sigh. @HRMPW @abayer I do feel strongly that we don't want to have |
@bitwiseman I'm not yet persuaded that parallel-at-top-level is a good idea. |
Wait, @bitwiseman, re-reading your original comment, I think you may have misunderstood - you can't do pipeline {
stages {
stage("foo") {
parallelStages {
stage("first") {
parallelStages {
stage("first-and-one") { just pipeline {
stages {
stage("foo") {
parallelStages {
stage("first") { i.e., a |
@abayer - Here's my core problem with the If |
@bitwiseman I disagree. The only thing that needs to be tested is that you can't use |
Well, ok, technically they're not identical behind the scenes in that in |
@abayer - Ah. I see. From a test perspective, I would never trust what you just said. 😃 How about let's compromise and add a test or two showing |
@bitwiseman Happy to do so! |
@bitwiseman And |
@rsandell @bitwiseman And now we have testing of nested stages with |
@abayer - Excellent! Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐝
if (thisStage.parallelStages != null) { | ||
def parallelStages = [:] | ||
for (int i = 0; i < thisStage.parallelStages.stages.size(); i++) { | ||
Stage parallelStage = thisStage.parallelStages.getStages().get(i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please stick to one syntax type, either parallelStages.stages
or parallelStages.getStages()
. I would prefer parallelStages.stages[i]
as well instead of parallelStages.stages.get(i)
Rebased on |
oh thats a shame, went with parallelStages vs just "parallel" (I guess it wasn't possible to repurpose the existing parallel function?) I thought the RFC originally said just "parallel" - the verbosity is not needed as it is clear there are stages below it - is it a technical constraint that it was named |
Will have to agree with @michaelneale here - this does not line up with the RFC so 🐜 from me too. |
@michaelneale @i386 I believe it is technically possible to call it just |
@i386 @michaelneale We discussed this offline a few weeks back? I'm still open to alternative names, but vehemently opposed to overloading |
This reverts commit 9e729c4.
d123b76
to
7707b42
Compare
@i386 @michaelneale FYI, I'm releasing 1.1.9 from master right now and then will be merging this to master, with the intention of not doing another release from master until Blue Ocean is ready for this. It's just been too damned long sitting unmerged. =) |
Conflicts: pipeline-model-api/pom.xml pipeline-model-definition/pom.xml pipeline-model-extensions/pom.xml pipeline-model-json-shaded/pom.xml pipeline-stage-tags-metadata/pom.xml pom.xml
@rtyler Not released yet! I just decided that it made no sense to sit unmerged any longer. I still owe a doc update PR too. |
@abayer - Still, every step toward this being merged is a win. |
stumbled upon this one yesterday as well, would love to see it being released 😄 |
@samgomena no, this only changes |
I've noticed that the stages are reported in different order sometimes which disruptes the pipeline stage view history. If stages within parallel were always reported in the order they are defined it would work better. This works great btw! Using it for multiple pipelines with great success! |
Morgan do you mind opening that as a new JIrA as I have seen that too. |
@kuhnroyal I have created an issue about not rendering properly skipped stages https://issues.jenkins-ci.org/browse/JENKINS-47219 |
stage
s. The backend implementation does not actually use thestage
step at this point, justparallel
branches, but the user experience with configuration, etc is the same.