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

When conditions based on changelog #178

Merged
merged 20 commits into from Aug 21, 2017

Conversation

Projects
None yet
5 participants
@rsandell
Copy link
Member

rsandell commented Aug 8, 2017

  • JENKINS issue(s):

  • Description:

    • Ability to skip stages based on what is in the changelog
      • changelog "regexp" - do a regexp check based on the changelog message
      • changeset "ant glob" - check based on what affected file paths are in the changeset
  • Documentation changes:

    • Will be needed
  • Users/aliases to notify:
    @reviewbybees @abayer

Downstream of jenkinsci/scm-api-plugin#46

When conditions based on changelog
Ability to skip stages based on what is in the changelog

changelog "regexp" - do a regexp check based on the changelog message
changeset "ant glob" - check based on what affected file paths are in the changeset

@rsandell rsandell requested a review from abayer Aug 8, 2017

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Aug 8, 2017

Needs some more tests and docs but uploaded to see if it is good in general.

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Aug 8, 2017

Could you open a JIRA for this as well? It's significant enough to merit one, I think.

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Aug 8, 2017

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.

@abayer
Copy link
Member

abayer left a comment

Looks good overall, mainly just nitpicks. =)

RunWrapper run = this.script.getProperty("currentBuild")
if (run != null) {
List<ChangeLogSet<? extends ChangeLogSet.Entry>> changeSets = run.getChangeSets()
for (int i = 0; i < changeSets.size(); i++) { //TODO can we do .each stuff yet?

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

Yes, we can. Well, more accurately, we can once #174 lands - or you could pick up the dependency changes in 120f9a4 (and the getPersistentAction removal in a5be631) to use 'em here and now with no problems.

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

Oh, and specifically, go with .any here - i.e., return changeSets.any { set -> set.any { change -> matches(change) } }

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 18, 2017

Author Member

done

//But it's probably simpler to make a build step that recaptures that information

if (gitChangeSetClass != null && change?.getClass()?.isAssignableFrom(gitChangeSetClass)) {
script.echo "We are running git"

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

I'd like a better message here - not "running" but maybe more like "Checking git changesets"?

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 18, 2017

Author Member

done

return describable.pattern.matcher(change.title).matches() || describable.multiLinePattern.matcher(change.comment).matches()
} else {
//Something generic
script.echo "We are running something generic"

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

See above - maybe "Checking general SCM changesets"?

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 8, 2017

Author Member

I didn't intend for those to be there, they where only there while debugging a test case.
But I'll add something based on your feedback.

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

We don't need something there - if that wasn't intended to be in the final version anyway, then feel free to not log anything.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 18, 2017

Author Member

done


@Override
boolean matches(ChangeLogSet.Entry change) {
def iterator = change.affectedPaths.iterator()

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

Switch to .any, I'd say.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 18, 2017

Author Member

done


@Override
void initializeEval() {
glob = (String)script.evaluate(Utils.prepareForEvalToString(describable.glob))

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

Throw a TODO here (and anywhere else there's script.evaluate and/or Utils.prepareForEvalToString) to update once #174 lands, so I don't forget 'em.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 9, 2017

Author Member

I couldn't remember exactly why you put them in there in the first place, so I just blindly copied from some other conditional. My guess was 'GString's :)

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 18, 2017

Author Member

done #174 has landed and the code has adapted.

initializeEval()
RunWrapper run = this.script.getProperty("currentBuild")
if (run != null) {
List<ChangeLogSet<? extends ChangeLogSet.Entry>> changeSets = run.getChangeSets()

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

Wondering if there's a way we can determine whether we've done a checkout scm already and report when we haven't...at the very least, probably makes sense to check changeSets.isEmpty() and report that there are no changes as well as returning false.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 9, 2017

Author Member

Yes, it is a bit sad. Even for multi-branch PRs the first build doesn't get a changelog :'(

I fix that in Gerrit Trigger by forcing a changelog in the BuildChooser. Maybe something similar can be fixed in github branch source.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 9, 2017

Author Member

Created JENKINS-46086 that I can hope being fixed some time.

*
* The build must first have collected the changelog via for example <code>checkout scm</code>.
*/
public class ChangesetConditional extends DeclarativeStageConditional<ChangesetConditional> {

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

Little nit - we tend to go with ChangeSet rather than Changeset, so let's be consistent here.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 9, 2017

Author Member

English and its wonderful inconsistencies :)

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 9, 2017

Author Member

Well that is actually not the word I want to be consistent with. I want to be consistent with the words changelog and changeset. So I'm actually keeping the camel case consistent here since the script and the describable follows the symbol name.
If I were to actually be consistent I would need to change the symbol names to changeLog and changeSet.

This comment has been minimized.

Copy link
@abayer

abayer Aug 9, 2017

Member

I'm more wed to keeping the class names consistent with other class and method names than having the symbols consistent with the class names, fwiw.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 18, 2017

Author Member

done


@DataBoundConstructor
public ChangelogConditional(String pattern) {
//TODO find a way to validate the regexp in the compile phase

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

Interesting! I'm about to work on a PR for providing additional validation checks by extension point - that'd probably be the right way to handle this rather than hardcoding it into ModelValidatorImpl.

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

fyi, JENKINS-46065 is in the works for that.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 18, 2017

Author Member

done

*
*/

pipeline {

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

Can we get JSON versions of these committed as well? And move these to src/test/resources, adding them to the list of test cases to roundtrip in AbstractModelDefTest...

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 9, 2017

Author Member

No, not moving them, there are too many resources in the root directory making it very hard to find what you need, grouping them by functionality or test suite makes it much simpler to find.

This comment has been minimized.

Copy link
@abayer

abayer Aug 9, 2017

Member

Boo. I mean, understandable, but boo. Mainly 'cos that makes hooking into the ConvertRoundTrip etc tests hairier. And that's a definite requirement here.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 9, 2017

Author Member

I am doing the json roundtrip part, the test code seems to be able to handle sub directories just fine :)

This comment has been minimized.

Copy link
@abayer

abayer Aug 9, 2017

Member

I must be missing something - where's that?

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 10, 2017

Author Member

"I'm doing" as in I'm working on it, not pushed since I'm having issues with the converter so tests are having issues with the regexp string.

Which means I might not push it until after the new parsing engine is merged.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 18, 2017

Author Member

done

}
stage("Two") {
when {
changelog '.*^\\[DEPENDENCY\\] .+$' //Perhaps we should use the /../ syntax directly so we don't suffer from ugly escaping?

This comment has been minimized.

Copy link
@abayer

abayer Aug 8, 2017

Member

Interesting - I'll need to see how that works in the #174 parser. Throw a TODO somewhere about that?

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 9, 2017

Author Member

There might be some issues with the describable mapping as well so I haven't tried to see if it works in this version. It would mean to take a Pattern instance as a @DataboundConstructor parameter.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 18, 2017

Author Member

I'll skip this for now, to much other stuff on the plate.

//But it's probably simpler to make a build step that recaptures that information

if (gitChangeSetClass != null && change?.getClass()?.isAssignableFrom(gitChangeSetClass)) {
script.echo "We are running git"

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 8, 2017

Author Member

doh! missed removing some debug prints

@abayer abayer added this to the 1.2 milestone Aug 8, 2017

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Aug 8, 2017

FYI, I'm not going to merge this until after #174 lands - we could rebase this on top of #174, but I don't think (other than the prepareForEvalToString usage) that there's anything of note that will be changed. Nonetheless, since there is that usage (and prepareForEvalToString vanishes in #174 since it's no longer needed), it makes sense to hold off.

That said, hopefully #174 will get reviewed soon. =)

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Aug 9, 2017

As long as It's in for Jenkins World, or at least I can say with confidence that it will be in soon at JW ;)

@rsandell rsandell requested a review from jglick Aug 15, 2017

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Aug 15, 2017

@jglick I'm mostly just curious on your feedback on 465eea3

//TODO JENKINS-46065 validate the regexp when #179 is merged
this.pattern = Pattern.compile(pattern);
this.multiLinePattern = Pattern.compile("(?m)(?s)^[^\\r\\n]*?" + pattern + "[^\\r\\n]*?$",
Pattern.MULTILINE | Pattern.DOTALL);

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

You do not need to both specify these flags as Java constants and inline in the regexp.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 17, 2017

Author Member

Well, this I just copied from the parts @abayer wrote in the build failure analyzer plugin, I honestly don't know exactly how everything works, just that it does :)

def branch = branchJobProperty.getBranch()
/*
Some special handling for pull requests to take into consideration all the builds for a particular PR.
Since a PR is a series of changes that will be merged in some way as one unit so all the changes should be considered.

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

JENKINS-33274 proposes a proper way of handling this.

RunWrapper run = this.script.getProperty("currentBuild")
if (run != null) {
List<ChangeLogSet<? extends ChangeLogSet.Entry>> changeSets = []
def branchJobProperty = run.rawBuild.parent.getProperty(BranchJobProperty.class) //TODO is there an easier way of finding this?

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

I think you meant to use SCMHead.HeadByItem.findHead.

if (branch.head.class.isAssignableFrom(bitbucketPr)) {
includeAllBuilds = true
}
} catch (ClassNotFoundException _) { /*Ignore*/

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

All this can be replaced with head instanceof ChangeRequestSCMHead.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 16, 2017

Author Member

Not if you read my comment, but could potentially be when JENKINS-33274 is implemented

void initializeEval() {
//Probably running with git plugin
try {
gitChangeSetClass = Class.forName("hudson.plugins.git.GitChangeSet")

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

Eek…

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 16, 2017

Author Member

I know :$


sampleRepo.write("webapp/js/somecode.js", "//fake file");
sampleRepo.git("add", "webapp/js/somecode.js");
sampleRepo.git("commit", "--message=files");

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

or consider FakeChangeLogSCM

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 16, 2017

Author Member

done


static void fakePRMarker(WorkflowJob job) throws NoSuchMethodException, IllegalAccessException, InvocationTargetException, InstantiationException, IOException {
//Black magic ahead!!
final Constructor<PullRequestSCMHead> pullRequestSCMHeadConstructor = PullRequestSCMHead.class.getDeclaredConstructor(String.class, boolean.class, int.class, BranchSCMHead.class, String.class, String.class, String.class);

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

Yuck, just use for example jenkins.scm.impl.mock.MockChangeRequestSCMHead.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 16, 2017

Author Member

Gah!
I've spent most of the day rewriting the tests to use MockSCMSource and then it turns out that it doesn't support affectedPaths in the changelog. sigh.

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 16, 2017

Author Member

Fixed

rsandell and others added some commits Aug 16, 2017

Add test comparing original groovy to original json
I got confused by something and forgot that we didn't have this in the
first place. Had to update a few json files to match due to changes in
the groovy files since the json was created. This should keep us from
hitting a few weird regressions in the future.
Disabling mismatched JSON quotes test
Ha, since we now double-up backslashes from the AST/JSON when we
convert to Groovy, this is no longer actually a problem - `"hello\\'"`
in JSON becomes `'hello\\''` in Groovy!

rsandell added some commits Aug 18, 2017

[JENKINS-46064] Groovy is groovy again
Use collection.any instead of boring traditional iterations
@@ -26,12 +26,15 @@
package org.jenkinsci.plugins.pipeline.modeldefinition.when.impl;

import hudson.Extension;
import hudson.RestrictedSince;

This comment has been minimized.

Copy link
@abayer

abayer Aug 18, 2017

Member

Unused?

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 18, 2017

Author Member

yes, some code complete mishap I think

This comment has been minimized.

Copy link
@rsandell

rsandell Aug 18, 2017

Author Member

fixed

@abayer

abayer approved these changes Aug 18, 2017

@abayer abayer merged commit c9683ac into master Aug 21, 2017

1 check passed

continuous-integration/jenkins/branch This commit looks good
Details
@casz

This comment has been minimized.

Copy link
Member

casz commented Jan 25, 2018

@rsandell I don't think this was ever documented in the docs https://jenkins.io/doc/book/pipeline/syntax/#when

@rsandell rsandell deleted the when-changelog branch Jan 25, 2018

@abayer

This comment has been minimized.

Copy link
Member

abayer commented Jan 25, 2018

@rsandell There should also be docs on jenkins.io - those aren't pulled from the in-plugin help text.

@rsandell

This comment has been minimized.

Copy link
Member Author

rsandell commented Jan 25, 2018

I guess I assumed jenkins-infra/pipeline-steps-doc-generator#11 was merged and made that work :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.