[FIX-JENKINS-38805_User_can_run_parametrised_pipeline] #692

scherler · 2017-01-10T19:10:07Z

Description

extending the RunButton to be able to start parametrised jobs where ever the "old" RunButton has placed
Refactor parameter implementation to use mobx
move parameter into core

state of the nation

The feature is ready to review and playing around.

ATH coverage

Current master ATH run on the branch https://ci.blueocean.io/job/ATH-Jenkinsfile/job/master/894/
Current branch ATH https://ci.blueocean.io/job/ATH-Jenkinsfile/job/master/919/

Next steps

create ATH to cover the basic steps with input parameter

Submitter checklist

Link to JIRA ticket in description, if appropriate.
Change is code complete and matches issue description
Appropriate unit or acceptance tests or explanation to why this change has no tests
Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.
Ran Acceptance Test Harness against PR changes.

Reviewer checklist

Run the changes and verified the change matches the issue description
Reviewed the code
Verified that the appropriate tests have been written or valid explanation given

@reviewbybees

…c version that works with input parameter, still have some real rough edges to fix

cliffmeyers · 2017-01-10T19:44:48Z

blueocean-dashboard/src/main/js/components/InputParameters.jsx

+    changeParameter(index, event) {
+        // console.log('onChange', index, event);
+        const originalParameters = this.state.parameters;
+        originalParameters[index].defaultParameterValue.value = event;


Per discussion in Gitter, it looks like the value property is observable here, so modifying it outside of @action decorator or action() function will throw this error when mobx useStrict(true) has been set. I believe that is set globally in core-js right now.

This may seem annoying but in a way I think it's good because it makes it very explicit when state change happens. Observable / data binding can be a slippery slope whenever any code can modify the data when there are shared references.

If you refer to core-js ToastService that's a very simple example where modification of an array is managed by two different @action methods. I think this is a good practice so that we're explicit and consistent of where we do state mutations.

On the other hand, if I understand the code correctly, you are pulling metadata about the input parameters from a job, then rendering them dynamically for user modification when launching a run. To me, you don't want to modify the underlying job / pipeline data in the data model during this user interaction. So I think what you probably want to do is clone the data here that drives the input parameters. If you still need advantages of observability and UI data binding, I write a quick class which encapsulates the form state and is build from a clone of the pipeline data. That way this data can be modified freely without changing the underlying data model.

michaelneale · 2017-01-11T04:15:44Z

great to see this stuff kick off !

i386 · 2017-01-11T04:21:51Z

blueocean-dashboard/src/main/js/components/InputParameters.jsx

+        const cancelButton = (<a title={cancelCaption} onClick={() => this.cancelForm()} className="btn inputStepCancel run-button btn-secondary" >
+            <span className="button-label">{cancelCaption}</span>
+        </a>);
+        return (<ModalView


@scherler is this the dialog component that @sophistifunk built or a new impl of the Modal? I believe we should be using the dialog not a new component (cc @brody) See https://issues.jenkins-ci.org/browse/JENKINS-39371

@i386 that is Modal view from jdl. I was not aware about the dialog component. will use that instead.

michaelneale · 2017-01-11T04:31:58Z

blueocean-dashboard/src/main/js/components/InputParameters.jsx

+ * In case you want to register a new mapping you need to edit './parameter/index' to add a new mapping
+ * and further in './parameter/commonProptypes' you need to include the new type in the oneOf array.
+ */
+export default class InputParameters extends Component {


tiny thing, but I worry that the name "Input" may confuse people looking at code for input vs params (they are very similar). It says parameters so this is ok... perhaps "ParameterForm" or something? just a suggestion. Nice commentary ;)

noted, the whole thing needs a second iteration to remove some code redundancies. I created the PR to get feedback from cliff.

i386 · 2017-01-11T12:40:18Z

Thanks Thor :)

…rking version with Dialog component and mobx based ParameterService

…ake it more generic and reusable

… to use the ParameterService

…peline

michaelneale · 2017-01-12T05:07:46Z

blueocean-dashboard/src/main/js/components/Activity.jsx

-                  onNavigation={onNavigation}
-                  innerButtonClasses="btn-secondary"
-                />
+                    <ParametersRunButton


So this contains the regular run button if I read it correctly.
This is maybe a little bit confusing name wise - it probably still should be "RunButton" but then it has components in it for normal and parametrised run button I think.

@michaelneale yes it contains the regular RunButton and shows it when no parameters are detected.

Regarding naming ParametersAwareRunButton had been to long but that would be the most specific name.

michaelneale · 2017-01-12T05:12:23Z

blueocean-dashboard/src/main/js/components/InputStep.jsx

@@ -114,7 +85,7 @@ export default class InputStep extends Component {
                        error.response = response;
                        throw error;
                    }
-                    return response;
+                    return response.json();


curious about this change - how did it work before?

michaelneale · 2017-01-12T09:33:03Z

good progress, did a quick look:

Also, in that case, @brody @i386 - should it not be "abort" but "cancel" or "close" as at that point it isn't actually running, so abort doesn't make as much sense (I don't know what the design showed).

i386 · 2017-01-13T05:58:48Z

Few things here @scherler:

That has to be Cancel instead of Abort
The dialog is too thin. @brody can you please provide guidance here?
I realise that there is a z-index problem that is creating the line across the dialog. This needs to be fixed in this PR or at least immediately after.
The dialog type you are using will overflow the content area of the dialog and always show the buttons. Is that how you are using it? I believe @sophistifunk allows the dialog to show buttons in a standard way and we should be using that.

brody · 2017-01-13T06:19:04Z

@scherler the design in zeplin is 464px wide. At least make it 450px wide and it'll be beautiful.

scherler · 2017-01-13T10:09:57Z

The dialog type you are using will overflow the content area of the dialog and always show the buttons. Is that how you are using it? I believe @sophistifunk allows the dialog to show buttons in a standard way and we should be using that.

I am using it as shown in the related story: https://github.com/jenkinsci/jenkins-design-language/blob/master/src/js/stories/DialogStories.jsx#L58

https://github.com/jenkinsci/blueocean-plugin/pull/692/files#diff-e999489b5e8809d41a4a6ba443345688R134 meaning as I understand I am using it in the standard way.

scherler · 2017-01-13T10:11:48Z

I am starting now to make it more appealing.

…peline

… API for submitting the forms

…sion of core-js

…ive more structure

… sure we start blank

scherler · 2017-01-13T12:53:43Z

@brody @i386 current stand

… issues

…core and extract input parameter from runnable. exporting the parameterRunButton as RunButton so all current usage support parameters out of the box. Tweak design of favorites to support parameterized starts.

…re-js - published under tag thor

ghost · 2017-01-14T11:12:19Z

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.

i386 · 2017-01-15T08:57:44Z

@scherler the "Build" button should be "Run"

michaelneale · 2017-01-16T03:45:56Z

Getting some test failures @scherler - worth looking into

michaelneale · 2017-01-16T04:00:42Z

blueocean-dashboard/src/main/js/components/Activity.jsx

-                        })
-                    }
-                </Table>
+                { runs.length > 0 &&


Is there any real diff here? It looks like formatting only, but its a dozen odd lines so hard to see - is there some tool doing that as it is a bit annoying as git can't tell it apart from real refactoring.

michaelneale · 2017-01-16T04:01:04Z

blueocean-dashboard/src/main/js/components/Activity.jsx

-                <button disabled={this.pager.pending || !this.pager.hasMore} className="btn-show-more btn-secondary" onClick={() => this.pager.fetchNextPage()}>
-                    {this.pager.pending ? t('common.pager.loading', { defaultValue: 'Loading...' }) : t('common.pager.more', { defaultValue: 'Show more' })}
-                </button>
+                { runs && runs.length > 0 &&


ditto with whitespace only changes???

yeah sorry about that.

michaelneale · 2017-01-16T07:29:07Z

very nice - working great so far @scherler!

…peline

…change Caption

…on in 4 language

alecharp

French translation advices

alecharp · 2017-01-16T11:37:57Z

blueocean-web/src/main/resources/jenkins/plugins/blueocean/web/Messages_fr.properties

@@ -3,6 +3,9 @@ login=Connexion
 logout=D\u00e9connexion
 Not.found.heading=Page non trouv\u00e9e (404)
 Not.found.message=Jenkins n'a pas pu trouver la page que vous recherchiez. V\u00e9rifiez l'URL pour des erreurs ou appuyez sur le bouton de retour. 
+parametrised.pipeline.cancel=Annuler
+parametrised.pipeline.header=Input indispensable
+parametrised.pipeline.submit=Lancer


I would say "Démarrer"

alecharp · 2017-01-16T11:39:47Z

blueocean-web/src/main/resources/jenkins/plugins/blueocean/web/Messages_fr.properties

@@ -3,6 +3,9 @@ login=Connexion
 logout=D\u00e9connexion
 Not.found.heading=Page non trouv\u00e9e (404)
 Not.found.message=Jenkins n'a pas pu trouver la page que vous recherchiez. V\u00e9rifiez l'URL pour des erreurs ou appuyez sur le bouton de retour. 
+parametrised.pipeline.cancel=Annuler
+parametrised.pipeline.header=Input indispensable


I would say "Entrée(s) indispensable" as "Input" is not a true French word. But the job configuration use the terms "parameters" so we could say "Paramètre(s) indispensable", which would be better for the configuration -> usage understanding.

…tions from feedback from Adrien.

alecharp

I think it's better this way.

…peline

…for translation

scherler · 2017-01-16T21:37:00Z

@michaelneale IMO ready for prime time. see jenkinsci/blueocean-acceptance-test#100 and passing https://ci.blueocean.io/job/ATH-Jenkinsfile/job/master/919/

…peline

…ion of workflow to for the input parameters

…ix workflow dependency to be able to use parameterized inputs

scherler · 2017-01-17T14:59:51Z

https://ci.blueocean.io/blue/organizations/jenkins/blueocean/detail/JENKINS-3.pkuu4orao08q.-pipeline/25/pipeline passed

scherler · 2017-01-17T15:31:00Z

https://ci.blueocean.io/job/ATH-Jenkinsfile/job/master/944 passed

[JENKINS-38805_User_can_run_parametrised_pipeline] WIP implement basi…

d638a03

…c version that works with input parameter, still have some real rough edges to fix

scherler added the work-in-progress label Jan 10, 2017

cliffmeyers reviewed Jan 10, 2017

View reviewed changes

i386 reviewed Jan 11, 2017

View reviewed changes

michaelneale reviewed Jan 11, 2017

View reviewed changes

[JENKINS-38805_User_can_run_parametrised_pipeline] WIP stabilze feature

aed0115

scherler added 5 commits January 11, 2017 22:09

[JENKINS-38805_User_can_run_parametrised_pipeline] Implement first wo…

facd2e4

…rking version with Dialog component and mobx based ParameterService

[JENKINS-38805_User_can_run_parametrised_pipeline] Refactor code to m…

f90e310

…ake it more generic and reusable

[JENKINS-38805_User_can_run_parametrised_pipeline] Refactor inputStep…

b64ecc6

… to use the ParameterService

Merge branch 'master' into JENKINS-38805_User_can_run_parametrised_pi…

742c2fe

…peline

[JENKINS-38805_User_can_run_parametrised_pipeline] remove obsolete code

0242e10

michaelneale reviewed Jan 12, 2017

View reviewed changes

[JENKINS-38805_User_can_run_parametrised_pipeline] Using core fetch lib

d99477e

scherler added 5 commits January 13, 2017 11:15

Merge branch 'master' into JENKINS-38805_User_can_run_parametrised_pi…

173265c

…peline

[JENKINS-38805_User_can_run_parametrised_pipeline] refactor to use an…

27e5638

… API for submitting the forms

[JENKINS-38805_User_can_run_parametrised_pipeline] Use merged new ver…

903f54a

…sion of core-js

[JENKINS-38805_User_can_run_parametrised_pipeline] refactor code to g…

de4dccd

…ive more structure

[JENKINS-38805_User_can_run_parametrised_pipeline] new method to make…

ae56998

… sure we start blank

scherler added 2 commits January 13, 2017 13:55

[JENKINS-38805_User_can_run_parametrised_pipeline] Fix various Design…

8774554

… issues

eslint - formating changes and fix offences

754db53

scherler added the needs-review label Jan 13, 2017

scherler added 2 commits January 14, 2017 01:40

[JENKINS-38805_User_can_run_parametrised_pipeline] move parameter to …

afa310b

…core and extract input parameter from runnable. exporting the parameterRunButton as RunButton so all current usage support parameters out of the box. Tweak design of favorites to support parameterized starts.

[JENKINS-38805_User_can_run_parametrised_pipeline] bump version of co…

5a6695b

…re-js - published under tag thor

scherler changed the title ~~[JENKINS-38805_User_can_run_parametrised_pipeline] WIP implement basi…~~ [FIX-JENKINS-38805_User_can_run_parametrised_pipeline] Jan 14, 2017

michaelneale reviewed Jan 16, 2017

View reviewed changes

scherler added 5 commits January 16, 2017 11:13

Merge branch 'master' into JENKINS-38805_User_can_run_parametrised_pi…

9d4acf6

…peline

[JENKINS-38805_User_can_run_parametrised_pipeline] fix tests

efe3b03

[JENKINS-38805_User_can_run_parametrised_pipeline] Fix proptypes and …

69101b1

…change Caption

[JENKINS-38805_User_can_run_parametrised_pipeline] add i18n translati…

2a5bbe1

…on in 4 language

[JENKINS-38805_User_can_run_parametrised_pipeline] fix typo

bd525c7

alecharp reviewed Jan 16, 2017

View reviewed changes

[JENKINS-38805_User_can_run_parametrised_pipeline] Fix french transla…

39d71d3

…tions from feedback from Adrien.

alecharp approved these changes Jan 16, 2017

View reviewed changes

scherler added 5 commits January 16, 2017 13:37

[JENKINS-38805_User_can_run_parametrised_pipeline] update version

e2c546f

Merge branch 'master' into JENKINS-38805_User_can_run_parametrised_pi…

0d696a3

…peline

[JENKINS-38805_User_can_run_parametrised_pipeline] add default value …

ac8f15a

…for translation

[JENKINS-38805_User_can_run_parametrised_pipeline] bumb version

9b68f3d

[JENKINS-38805_User_can_run_parametrised_pipeline] fix shrinkcrap

3ba20b8

scherler added 3 commits January 17, 2017 14:46

Merge branch 'master' into JENKINS-38805_User_can_run_parametrised_pi…

3c41e09

…peline

[JENKINS-38805_User_can_run_parametrised_pipeline] We need newer vers…

8cdf0b7

…ion of workflow to for the input parameters

[JENKINS-38805_User_can_run_parametrised_pipeline] bumb version and f…

8cc8f10

…ix workflow dependency to be able to use parameterized inputs

scherler merged commit d873acc into master Jan 17, 2017

scherler deleted the JENKINS-38805_User_can_run_parametrised_pipeline branch January 17, 2017 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX-JENKINS-38805_User_can_run_parametrised_pipeline] #692

[FIX-JENKINS-38805_User_can_run_parametrised_pipeline] #692

scherler commented Jan 10, 2017 •

edited

Loading

cliffmeyers Jan 10, 2017 •

edited

Loading

michaelneale commented Jan 11, 2017

i386 Jan 11, 2017 •

edited

Loading

scherler Jan 11, 2017

michaelneale Jan 11, 2017

scherler Jan 11, 2017 •

edited

Loading

i386 commented Jan 11, 2017

michaelneale Jan 12, 2017

scherler Jan 13, 2017

michaelneale Jan 12, 2017

michaelneale commented Jan 12, 2017

i386 commented Jan 13, 2017

brody commented Jan 13, 2017

scherler commented Jan 13, 2017

scherler commented Jan 13, 2017

scherler commented Jan 13, 2017 •

edited

Loading

ghost commented Jan 14, 2017

i386 commented Jan 15, 2017

michaelneale commented Jan 16, 2017

michaelneale Jan 16, 2017

michaelneale Jan 16, 2017

scherler Jan 16, 2017

michaelneale commented Jan 16, 2017

alecharp left a comment

alecharp Jan 16, 2017

alecharp Jan 16, 2017

alecharp left a comment

scherler commented Jan 16, 2017

scherler commented Jan 17, 2017

scherler commented Jan 17, 2017

[FIX-JENKINS-38805_User_can_run_parametrised_pipeline] #692

[FIX-JENKINS-38805_User_can_run_parametrised_pipeline] #692

Conversation

scherler commented Jan 10, 2017 • edited Loading

Description

state of the nation

ATH coverage

Next steps

Submitter checklist

Reviewer checklist

cliffmeyers Jan 10, 2017 • edited Loading

Choose a reason for hiding this comment

michaelneale commented Jan 11, 2017

i386 Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scherler Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

i386 commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelneale commented Jan 12, 2017

i386 commented Jan 13, 2017

brody commented Jan 13, 2017

scherler commented Jan 13, 2017

scherler commented Jan 13, 2017

scherler commented Jan 13, 2017 • edited Loading

ghost commented Jan 14, 2017

i386 commented Jan 15, 2017

michaelneale commented Jan 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelneale commented Jan 16, 2017

alecharp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecharp left a comment

Choose a reason for hiding this comment

scherler commented Jan 16, 2017

scherler commented Jan 17, 2017

scherler commented Jan 17, 2017

scherler commented Jan 10, 2017 •

edited

Loading

cliffmeyers Jan 10, 2017 •

edited

Loading

i386 Jan 11, 2017 •

edited

Loading

scherler Jan 11, 2017 •

edited

Loading

scherler commented Jan 13, 2017 •

edited

Loading