[JENKINS-38153] Adding synthetic stage marker. #13

abayer · 2016-09-13T01:07:32Z

Also adding synthetic stages for pre-build stuff.

cc @reviewbybees esp @vivek

Also adding synthetic stages for pre-build stuff.

ghost · 2016-09-13T01:14:02Z

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 · 2016-09-13T16:32:38Z

Also cc @svanoort

svanoort · 2016-09-13T16:56:41Z

Agree with adding a new action -- however I think this is where we should do a trivial JENKINS-26522 implementation. Give me a minute I'm going to hack together something for you :)

"Synthetic" would be one of the annotations for us, just one of many, used here.

Need a bit of time in order to be able to give you a full review.

svanoort · 2016-09-13T17:30:24Z

@abayer and @vivek I like the basic ideal but think a JENKINS-26522 implemetation around arbitrary annotations like this is better and more flexible:

jenkinsci/workflow-api-plugin#12 <-- POC concept

In that case, we'd use a tag "SYNTHETIC" on the action.

abayer · 2016-09-13T17:34:08Z

@svanoort Well, that's part of it, but we did need more metadata than just the tag - we're also recording the execution context (currently the only options are pre-build/post-build, but I have a feeling we'll add more to that in the future).

svanoort · 2016-09-13T17:53:48Z

execution context

Yeah you're just annotating which synthetic stage type it is. That's a SYNTHETIC tag and/or SYNTHETIC_PRE/SYNTHETIC_POST.

Are there additional requirements/plans that have not been disclosed yet that you'd like to disclose now?

abayer · 2016-09-13T18:05:40Z

Awwww, I have to translate everything to string tags? I'm back in AWS-land! =)

svanoort · 2016-09-13T19:09:40Z

@abayer Okay, but implementation-nitpicking aside: don't tell me you don't want JENKINS-26522... ;) Or that you will get to be the trailblazer use case to justify it, with pipeline model definitions.

abayer · 2016-09-13T19:11:15Z

Yes, I do want that. =)

Conflicts: src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy

jglick · 2016-09-14T14:21:54Z

Seems hackish. Better to define a simple Step with a block and whatever Actions you need, possibly though not necessarily including LabelAction.

svanoort · 2016-09-16T16:11:25Z

@abayer If we pull in the TagsAction here I think will be ready for merge?

abayer · 2016-09-16T17:35:30Z

@svanoort Yeah, I can redo this with TagsAction.

abayer · 2016-09-19T17:35:59Z

Now properly downstream of jenkinsci/workflow-api-plugin#12

Conflicts: src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java

abayer · 2016-09-19T17:52:09Z

FYI, current failure on ci.jenkins.io PR build is due to SNAPSHOT check staleness.

svanoort · 2016-09-19T17:49:23Z

src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/actions/SyntheticContext.java

+
+package org.jenkinsci.plugins.pipeline.modeldefinition.actions;
+
+public enum SyntheticContext {


🐛 I think you meant to remove this, since we don't need the whole Context/ContextAction bit after adding the TagsAction.

Nope, I meant to leave it - I want an enum of possible values for the SYNTHETIC_STAGE_TAG.

..but have now gotten rid of it. yay?

svanoort · 2016-09-19T17:50:07Z

.../java/org/jenkinsci/plugins/pipeline/modeldefinition/actions/SyntheticStageMarkerAction.java

+ *
+ * @author Andrew Bayer
+ */
+public class SyntheticStageMarkerAction extends InvisibleAction {


🐛 Not needed now that we can use TagsAction as our marker (and indeed a broader-use marker).

Yeah, forgot to delete that.

svanoort · 2016-09-19T17:53:19Z

src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy

+     * @param context
+     */
+    @Whitelisted
+    static void markSyntheticStage(String stageName, SyntheticContext context) {


🐛 Better to use static final strings for the markers, i.e. "PRE" and "POST" or similar. Creating an enum just for two values is kind of silly, and if we're just getting string values for 'em we might as well use that.

Sigh. Ok. =)

svanoort · 2016-09-19T17:55:23Z

src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java

+        assertNull(scanner.findFirstMatch(heads, null, syntheticStagePredicate(SyntheticStageNames.dockerPull(), SyntheticContext.PRE)));
+    }
+
+    private Predicate<FlowNode> syntheticStagePredicate(final String stageName,


Would it make sense to expose this as part of the public API for Blue Ocean to use with the search/filter workflow APIs?

I'm betting y'all can write something better. =)

You give people too much credit ;)

abayer · 2016-09-19T18:09:42Z

@svanoort Ok, switched to Strings. =)

jglick · 2016-09-19T18:19:11Z

src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy

+        if (currentNode.getAction(TagsAction.class) == null) {
+            currentNode.actions.add(new TagsAction())
+        }
+        currentNode.getAction(TagsAction.class).addTag(SYNTHETIC_STAGE_TAG, context)


This is a hack. Better to have a block-scoped Step which adds this Action using the supported APIs.

Is there a way to limit the visibility/exposure of a Step? i.e., can I create one that only can be used in this plugin? If not, then hacks are really the only way I can see to get what we need - a Step that shows up in the snippet generator, etc that does this would be...flawed.

isAdvanced

Unfortunately I have to disagree with @jglick about the block-scoped step -- it doesn't feel like synthetic stages are truly a different animal than a normal stage. We're just attaching some additional helper information to that stage. Actions are exactly the right thing to use here.

I strangely enough agree with @svanoort =)

jglick · 2016-09-19T18:20:38Z

src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy

+                script.stage(SyntheticStage.dockerPull()) {
+                    Utils.markSyntheticStage(SyntheticStage.dockerPull(), SyntheticStage.SYNTHETIC_PRE)
+                    script.getProperty("docker").image(agent.docker).pull()
+                }
                script.getProperty("docker").image(agent.docker).inside {


BTW more directly written as

script.docker.image…

So at least at some point in the past, that very much didn't work. Dunno if it would now, I'll give it a try.

abayer · 2016-09-19T20:01:42Z

ci.jenkins.io is still...questionable at times. Right now it's barfing due to git user.name and user.email not being set. Fun.

svanoort · 2016-09-19T22:16:36Z

src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java

    }

    private Predicate<FlowNode> syntheticStagePredicate(final String stageName,
-                                                        final SyntheticContext context) {
+                                                        final String context) {
        return new Predicate<FlowNode>() {
            @Override
            public boolean apply(FlowNode input) {
                return input.getDisplayName().equals(stageName) &&
                        input.getAction(TagsAction.class) != null &&


🐛 Upon further examination (sorry). Fetching the action 3x is undesirable for performance (sigh). Normally pessimization wouldn't be a bug but I happen to know this one can be fairly costly.

Also worth checking for 'input instanceof StepNode && ((StepNode)input).getDescriptor == WhateverTheStageDescriptorIs' -- faster than string equals since it is a direct memory location compare.

Wait. Nevermind, that's test-only. It would be a bug if it were a public API.

Going to blame that one on still being jetlagged post-JenkinsWorld.

Also, I'm specifically trying to compare the displayName to make sure it's the right stage. I did try using StageStartNode, I think it was, whatever you suggested earlier, and it...did not work. At all. So I went back to this. =)

Display name can be the typeDisplayName if there isn't a LabelAction though, so it's not a safe check to be using in general (since you can get false positives). Again, okay in the walled garden of tests, but I wouldn't want to see it elsewhere.

In general, prefer type checks and then a LabelAction (or StageAction) check -- you're putting preconditions first that can be tested quickly. Should be safer and a bit faster (since it'll return negative 99% of the time).

svanoort · 2016-09-19T22:24:14Z

In general: the context removal and string changeover look good.

I think this one is 🐝 worthy -- though might suggest using your NotNull/CheckForNull annotations some more.

abayer · 2016-11-04T18:21:08Z

Closing in favor of #47

[JENKINS-38153] Adding synthetic stage marker.

cc963d0

Also adding synthetic stages for pre-build stuff.

Stop duplicating strings so dang much

7a33b25

Merge branch 'master' into synthetic-stages

582f225

Conflicts: src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy

svanoort mentioned this pull request Sep 14, 2016

Create a new TagsAction [JENKINS-26522] jenkinsci/workflow-api-plugin#12

Merged

abayer added 3 commits September 16, 2016 14:29

Merge branch 'master' into synthetic-stages

6e332cb

Merge branch 'master' into synthetic-stages

918c517

Merge branch 'master' into synthetic-stages

371fbdb

svanoort mentioned this pull request Sep 19, 2016

Revert "Create a new TagsAction [JENKINS-26522]" jenkinsci/workflow-api-plugin#14

Merged

Now downstream of jenkinsci/workflow-api-plugin#12

90d37a3

Merge branch 'master' into synthetic-stages

5152a99

Conflicts: src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BasicModelDefTest.java

abayer closed this Sep 19, 2016

abayer reopened this Sep 19, 2016

svanoort suggested changes Sep 19, 2016

View reviewed changes

abayer added 2 commits September 19, 2016 11:10

Remove SyntheticContext.

63cfb28

Accidentally deleted ExecutorModelAction oops

ec1a2a9

jglick reviewed Sep 19, 2016

View reviewed changes

svanoort reviewed Sep 19, 2016

View reviewed changes

abayer closed this Nov 4, 2016


		package org.jenkinsci.plugins.pipeline.modeldefinition.actions;

		public enum SyntheticContext {

[JENKINS-38153] Adding synthetic stage marker. #13

[JENKINS-38153] Adding synthetic stage marker. #13

Conversation

abayer commented Sep 13, 2016

ghost commented Sep 13, 2016

abayer commented Sep 13, 2016

svanoort commented Sep 13, 2016 • edited

svanoort commented Sep 13, 2016

abayer commented Sep 13, 2016

svanoort commented Sep 13, 2016 • edited

abayer commented Sep 13, 2016

svanoort commented Sep 13, 2016

abayer commented Sep 13, 2016

jglick commented Sep 14, 2016

svanoort commented Sep 16, 2016

abayer commented Sep 16, 2016

abayer commented Sep 19, 2016

abayer commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abayer commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svanoort Sep 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abayer commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svanoort commented Sep 19, 2016

abayer commented Nov 4, 2016

svanoort commented Sep 13, 2016 •

edited

svanoort commented Sep 13, 2016 •

edited

svanoort Sep 19, 2016 •

edited