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

[JENKINS-26107] Labeled block #1

Closed
wants to merge 3 commits into from

Conversation

@jglick
Copy link
Member

jglick commented Apr 8, 2016

Moved from jenkinsci/pipeline-plugin#140.

JENKINS-26107

@reviewbybees

@jglick jglick mentioned this pull request Apr 8, 2016
2 of 4 tasks complete
@jenkinsadmin

This comment has been minimized.

Copy link
Member

jenkinsadmin commented Apr 8, 2016

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@jglick jglick removed the work-in-progress label Apr 8, 2016
@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Apr 8, 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.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented Apr 12, 2016

@HRMPW notes that the term label is also used to classify and select agents, for example as a parameter to the node step, so its use here might be confusing. Is there an alternate term which would be intuitive as a step name, and not overloaded in a Jenkins context? segment?

/*
* The MIT License
*
* Copyright 2015 Jesse Glick.

This comment has been minimized.

Copy link
@amuniz

amuniz Apr 12, 2016

Member

2016?

@amuniz

This comment has been minimized.

Copy link
Member

amuniz commented Apr 12, 2016

🐝 - regardless the final name for this step.

@emanuelez

This comment has been minimized.

Copy link
Member

emanuelez commented Apr 20, 2016

FWIW I think the name label fits fine and I don't see a lot of confusion happening with node labels.
Looking forward to this! :)

@abayer

This comment has been minimized.

Copy link
Member

abayer commented May 4, 2016

I'd vote for segment or section or something like that over an already meaningful term like label.

@svanoort

This comment has been minimized.

Copy link
Member

svanoort commented May 4, 2016

@jglick Is there a reason we don't attach a LabelAction here?

Personally I like calling this a 'Tag' or 'NamedBlock.'

Point of note: I am realizing that there are some serious complexities in this and in mixing and matching stages and $WhateverWeCallThis. Stages are ordered by either iota or startTime and unscoped, but $ThisThing is scoped. I think we should also define how we expect those 'what contains what' issues to be handled.

A couple things to consider:

  • Due to the fact that this is attached to blocks (which can nest), we may nest labelled blocks, and indeed may mix named sub-blocks with atomic steps, i.e.:
label('top') {
  sh 'cp myFile newFile'
  label('more') {
    sh 'bash doMore.sh'  // Nested
      label('or_less') {  // Nested under or_less, more, then top
        sh 'less newFile'
      }
  }
  sh 'echo "we are done"'  // Nested under top
}
echo 'But wait there is more!'  //I have no label?
  • Evaluating whether a step executed as part of a stage or a labelled block is rather complex:
stage 'mine'
echo 'I am part of the stage.'
label('no, mine!') {
  echo 'Clearly, I belong to the label.  Maybe also the stage?'
}
echo 'but who owns me? stage, presumably.'

Let's not even consider the case where we have a stage preceding a parallel block, with labels mixed in.

We will obviously have to store the scopes of blocks in a stage as we walk the Flow graphs to evaluate where steps ran (not a big deal), and also the list of stage steps (to see if something happened before or during one).

Consider also: do we want to describe labels/tags/names as a list with all applicable values, from the outside in? No change needed in the step itself, I think, but only in how we evaluate it within UIs. This may actually be more useful and logical for users than seeing (for example) the nearest descriptive value. In addition it isn't the end of the world if the ordering is off sometimes. It also avoids the puzzling behavior (for users) of seeing that last echo step as part of a stage that doesn't appear to contain the preceding blocks (since they have separate labels applied).

Maybe I'm missing something in the intended use here?

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented May 4, 2016

Is there a reason we don't attach a LabelAction here?

Huh? We do.

Let's not even consider the case where we have a stage preceding a parallel block, with labels mixed in.

The intent in that case is clear to my mind. The code between one stage and the next serves as an implicit labeled block, enclosing the explicit ones. FlowNodeSerialWalker already implemented this.

The tricky case is stage inside some (non-parallel) labeled block; for that case I think we can simply say the behavior is undefined and leave it at that, since stage was only ever intended for top-level use.

Anyway with the advent of label you should just stop using stage anyway. There is no need to support scripts using both.

node {
  stage 'Check out'
  checkout scm
  stage 'Build'
  sh 'make'
  stash name: 'app', includes: 'app.bin'
}
node {
  stage 'Test'
  unstash 'so'
  sh './app.bin'
}

should be translated to something like

label('Dev') {
  node {
    label('Check out') {
      checkout scm
    }
    label('Build') {
      sh 'make'
      stash name: 'app', includes: 'app.bin'
    }
  }
}
label('Test') {
  node {
    unstash 'so'
    sh './app.bin'
  }
}

You could choose to leave the node blocks outside of label, in which case they would simply have no associated label, so any messages or pauses or metadata (for example, waiting for a free executor) would just be show at top level in some UI-specific manner, as if the entire build were wrapped in an implicit

label('<top level>') {
  //
}

This may actually be more useful and logical for users than seeing (for example) the nearest descriptive value.

No particular opinion. Different UIs may have different needs, and different amount of space available.

@jglick

This comment has been minimized.

Copy link
Member Author

jglick commented May 12, 2016

@kohsuke wants this as a mode of stage.

@jglick jglick closed this May 12, 2016
@jglick jglick deleted the jglick:label-JENKINS-26107 branch May 12, 2016
@abayer

This comment has been minimized.

Copy link
Member

abayer commented May 12, 2016

+1 to that. =)

@svanoort

This comment has been minimized.

Copy link
Member

svanoort commented May 12, 2016

It certainly reduces user confusion, but I'm wincing a bit thinking of how we'll handle pipelines with two modes of operation for stage (snapshot-in-time vs. block-scoped).

abayer added a commit that referenced this pull request Apr 18, 2018
Showing how to do Channel.export
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.