[FIXED JENKINS-42999] Allow bindings to specify their required context #34

abayer · 2017-03-21T22:33:46Z

Not all bindings require a workspace, and those that don't should be
able to be used in withCredentials outside of a node block. This
adds BindingDescriptor.getRequiredContext(), defaulting to the four
contexts that were previously required by BindingStep, but allowing
override. BindingStep will now throw a
MissingContextVariableException if any of the MultiBindings used
have a required context that cannot be satisfied, as well as the
normal potential MissingContextVariableException for BindingStep.DescriptorImpl.getRequiredContext().

In addition, bumped workflow-step-api to 2.9 and moved BindingStep
and friends to non-deprecated code paths while I was here.

cc @reviewbybees esp @jglick @stephenc @rsandell


        [FIXED JENKINS-42999] Allow bindings to specify their required context

Not all bindings require a workspace, and those that don't should be able to be used in `withCredentials` outside of a `node` block. This adds `BindingDescriptor.getRequiredContext()`, defaulting to the four contexts that were previously required by `BindingStep`, but allowing override. `BindingStep` will now throw a `MissingContextVariableException` if any of the `MultiBinding`s used have a required context that cannot be satisfied, as well as the normal potential `MissingContextVariableException` for `BindingStep.DescriptorImpl.getRequiredContext()`. In addition, bumped `workflow-step-api` to 2.9 and moved `BindingStep` and friends to non-deprecated code paths while I was here.

reviewbybees · 2017-03-21T22:36:28Z

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.

rsandell · 2017-03-22T10:25:18Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java

+
+            FilePath workspace = getContext().get(FilePath.class);
+            Launcher launcher = getContext().get(Launcher.class);
+            TaskListener listener = getContext().get(TaskListener.class);


why not fail quickly here as well since you claim to require a TaskListener below in getRequiredContext?

Good point, fixing.


        Fail fast on missing TaskListener

rsandell

🐝

jglick · 2017-03-23T21:28:28Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/BindingDescriptor.java

@@ -44,6 +54,19 @@

    protected abstract Class<C> type();

+    /**
+     * Returns the context {@link MultiBinding} needs to access. Defaults to {@link Run}, {@link FilePath},
+     * {@link Launcher} and {@link TaskListener}.


Oxford comma!!!

jglick · 2017-03-23T21:28:28Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java

+        return new Execution(this, context);
+    }
+
+    public static final class Execution extends StepExecution {


Need no longer be public.

jglick · 2017-03-23T21:28:28Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java

+        return new Execution(this, context);
+    }
+
+    public static final class Execution extends StepExecution {


For running-build compatibility you must continue to extend AbstractStepExecutionImpl; see the Javadoc for nondeprecated constructor.

Owie. Okiedokie.

jglick · 2017-03-23T21:28:28Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java


        @Override public boolean start() throws Exception {
+            Run<?,?> run = getContext().get(Run.class);
+            if (run == null) {


Impossible—we have declared this in getRequiredContext.

Yup, cleaning up.

jglick · 2017-03-23T21:28:28Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java

+                throw new MissingContextVariableException(Run.class);
+            }
+            TaskListener listener = getContext().get(TaskListener.class);
+            if (listener == null) {


jglick · 2017-03-23T21:28:28Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java

@@ -177,9 +206,14 @@ private Object readResolve() throws ObjectStreamException {

        @Override protected void finished(StepContext context) throws Exception {
            Exception xx = null;
+            Run<?,?> run = context.get(Run.class);
+            if (run == null) {


jglick · 2017-03-23T21:28:28Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java

+                    if (v == null) {
+                        throw new MissingContextVariableException(requiredContext);
+                    }
+                }
                MultiBinding.MultiEnvironment environment = binding.bind(run, workspace, launcher, listener);


Hmm. So MultiBinding.bind’s signature should be changed to make workspace and launcher @Nullable, with Javadoc explaining whether the implementation can or cannot rely on them being present. Same for Unbinder. (listener should be marked @Nonnull like build already was.)

Wait, @Nullable, not @CheckForNull?

Yes—whether or not an implementer actually needs to check for null depends on factors outside of the reasonable scope of static analysis: how they implemented another method.

Fair point!

jglick · 2017-03-23T21:28:28Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/BindingDescriptor.java

+     * (say in freestyle or in workflow).
+     * @see StepContext#get(Class)
+     */
+    public Set<? extends Class<?>> getRequiredContext() {


This does not really make sense as an API. There are two legitimate choices:

Run + TaskListener + FilePath + Launcher (the default)

Run + TaskListener (the new mode)

You could not usefully drop, say, Run or TaskListener—they will still be required. And you could not meaningfully request some additional context—you will indeed make the step fail if not called in that context, but you have no way of accessing the contextual object! (Since a MultiBinding.bind just gets fixed parameters.)

So I would just scrap this API and replace it with a simple

public boolean requiresWorkspace() { return true; }

Works for me.

jglick · 2017-03-23T21:28:28Z

src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStepTest.java

+                WorkflowJob p = story.j.jenkins.createProject(WorkflowJob.class, "p");
+                p.setDefinition(new CpsFlowDefinition(""
+                        + "withCredentials([usernamePassword(usernameVariable: 'USERNAME', passwordVariable: 'PASSWORD', credentialsId: '" + credentialsId + "')]) {\n"
+                        + "  node {\n"  // We still need to enter a node to get the actual creds via the file.


No need. Try something like

withCredentials([usernameColonPassword(variable: 'USERPASS', credentialsId: '…')]) { semaphore '…' echo THEVAR.toUpperCase() }

which should print BOB:S3CR3T.

Ah-ha - nice.

jglick · 2017-03-23T21:28:28Z

src/test/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStepTest.java

+                WorkflowRun b = p.scheduleBuild2(0).waitForStart();
+                story.j.assertBuildStatus(Result.FAILURE, story.j.waitForCompletion(b));
+                story.j.assertLogNotContains("We should fail before getting here", b);
+                // We can't guarantee whether this will fail due to missing FilePath.class or Launcher.class.


Well if you adopt my suggested API you can predict that.


        Addressing review comments

jglick · 2017-03-24T18:19:04Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java

-                        throw new MissingContextVariableException(requiredContext);
-                    }
+                if (binding.getDescriptor().requiresWorkspace() && workspace == null) {
+                    throw new MissingContextVariableException(FilePath.class);


Technically you should also check for Launcher, though in practice they will both be present or neither.

jglick · 2017-03-24T18:19:04Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/FileBinding.java

-    private static FilePath secretsDir(FilePath workspace) {
-        return tempDir(workspace).child("secretFiles");
+    @CheckForNull
+    private static FilePath secretsDir(@CheckForNull FilePath workspace) {


Revert. It will not be null.

jglick · 2017-03-24T18:19:04Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/FileBinding.java

-            // needs to be executable so we can list the contents
-            secret.chmod(0700);
+        if (secrets == null) {
+            throw new IOException("Can't proceed with null workspace");


Revert. It cannot be null.


        A few more review comments

jglick · 2017-03-30T18:03:05Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java

@@ -107,7 +107,8 @@ public StepExecution start(StepContext context) throws Exception {
            Map<String,String> overrides = new HashMap<String,String>();
            List<MultiBinding.Unbinder> unbinders = new ArrayList<MultiBinding.Unbinder>();
            for (MultiBinding<?> binding : step.bindings) {
-                if (binding.getDescriptor().requiresWorkspace() && workspace == null) {
+                if (binding.getDescriptor().requiresWorkspace() &&
+                        (workspace == null || launcher == null)) {
                    throw new MissingContextVariableException(FilePath.class);


Misleading in case FilePath but not Launcher is present; again this is only theoretical.

Yeah - I decided to go with FilePath as the one I errored on fairly arbitrarily.

jglick · 2017-03-30T18:03:05Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/BindingStep.java

@@ -177,6 +199,7 @@ private Object readResolve() throws ObjectStreamException {

        @Override protected void finished(StepContext context) throws Exception {
            Exception xx = null;
+


jglick · 2017-03-30T18:03:05Z

src/main/java/org/jenkinsci/plugins/credentialsbinding/impl/FileBinding.java

@@ -62,8 +69,7 @@
            // needs to be writable so we can delete its contents
            // needs to be executable so we can list the contents
            secret.chmod(0700);
-        }
-        else {
+        } else {


abayer mentioned this pull request Mar 21, 2017

[FIXED JENKINS-42858] Process credentials before rest of environment #141

Closed

rsandell reviewed Mar 22, 2017

View changes

Fail fast on missing TaskListener

Loading status checks…

0c0d474

rsandell approved these changes Mar 23, 2017

View changes

jglick requested changes Mar 23, 2017

View changes

Addressing review comments

Loading status checks…

6a0ba3d

jglick requested changes Mar 24, 2017

View changes

A few more review comments

Loading status checks…

062dba1

jglick approved these changes Mar 30, 2017

View changes

jglick merged commit d31ff50 into jenkinsci:master Mar 30, 2017
1 check passed

1 check passed

Jenkins This pull request looks good
Details

abayer mentioned this pull request Apr 19, 2017

[JENKINS-37324] Store parameter info for steps as an action on the flownode #98

Merged

jenkinsci / credentials-binding-plugin

[FIXED JENKINS-42999] Allow bindings to specify their required context #34

[FIXED JENKINS-42999] Allow bindings to specify their required context #34

abayer commented Mar 21, 2017

reviewbybees commented Mar 21, 2017

This comment has been minimized.

This comment has been minimized.

rsandell left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

		@@ -177,6 +199,7 @@ private Object readResolve() throws ObjectStreamException {

		@Override protected void finished(StepContext context) throws Exception {
		Exception xx = null;

jenkinsci / credentials-binding-plugin

Join GitHub today

[FIXED JENKINS-42999] Allow bindings to specify their required context #34

[FIXED JENKINS-42999] Allow bindings to specify their required context #34

Conversation

abayer commented Mar 21, 2017

reviewbybees commented Mar 21, 2017

This comment has been minimized.

This comment has been minimized.

rsandell left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.