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

Add build selector parameter #1096

Merged

Conversation

OlgaMaciaszek
Copy link
Contributor

This is a PR to allow using the Build Selector Parameter for the Copy Artifact Plugin (https://wiki.jenkins.io/display/JENKINS/Copy+Artifact+Plugin),

NOTE: I do know that an auto-generated method for that in theory exists, however, it does not work properly. Using it according to the API causes the following error:

ERROR: Found multiple extensions which provide method upstream with arguments [devskiller.services.ServicesJobBuilder$_configureDeployJob_closure4$_closure20$_closure25$_closure26$_closure27@12fb813d]: [[com.tikal.jenkins.plugins.multijob.MultiJobBuildSelector, hudson.plugins.copyartifact.TriggeredBuildSelector]]
Finished: FAILURE

The changes in this PR have been tested and they generate the parameter correctly.

Copy link
Member

@daspilker daspilker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also supported by the Automatically Generated DSL:

buildSelectorParameter {
    name(String value)
    defaultSelector {
        buildParameter {}
        downstream {}
        lastCompleted()
        lastSuccessful {}
        latestSavedBuild()
        permalink {}
        specific {}
        upstream {}
        workspace()
    }
    description(String value)
}

Either address the comments or close the PR if the generated DSL works for you.

* @since 1.47
*/
@PackageScope
Item getItem() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this supposed to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will return super.item. I wouldn't normally add it, but I have seen such method added in StepContext and thought you had some kind of convention in place for classes extending AbstractExtensibleContext. Should I just delete it?

*
* @param classifier String value of the classifier
*/
void classifier(String classifier) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this a constructor parameter

* @since 1.67
*/
@RequiresPlugin(id = 'copyartifact', minimumVersion = '1.31')
void buildSelectorParameter(String parameterName,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this to buildSelectorParam. All other methods in this context are named xxxParam, so we should keep the pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will change.

@OlgaMaciaszek
Copy link
Contributor Author

OlgaMaciaszek commented Feb 16, 2018

@daspilker Thanks for the review. Must have missed notifications. Will address the comments today.

@OlgaMaciaszek
Copy link
Contributor Author

@daspilker This auto-generated method does not work properly. I have enclosed info about that and error message in my first comment.

@OlgaMaciaszek
Copy link
Contributor Author

@daspilker I have added some changes. Please verify if this PR is ok now.

@daspilker
Copy link
Member

@OlgaMaciaszek ah, I somehow missed the problem with the auto generated DSL. It happens when copyartifact and multijob plugins are installed. This seems to be a bug in multijob or structs plugin. I filed the problem as JENKINS-49963.

Your PR looks good so far. Please add some tests to BuildParametersContextSpec.

@OlgaMaciaszek
Copy link
Contributor Author

@daspilker I'm really sorry for not getting back to you earlier. I must have missed the notification. Will add the tests tomorrow.

@OlgaMaciaszek
Copy link
Contributor Author

OlgaMaciaszek commented Jun 13, 2018

@daspilker I have added tests. Please have a look.

@@ -10,10 +10,13 @@ import javaposse.jobdsl.dsl.RequiresPlugin

@ContextType('hudson.plugins.copyartifact.BuildSelector')
class CopyArtifactSelectorContext extends AbstractExtensibleContext {

final String classifier
Copy link

@szpak szpak Jun 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for having it final

@szpak
Copy link

szpak commented Jun 23, 2018

@daspilker https://issues.jenkins-ci.org/browse/JENKINS-49963 has been closed automatically. Wouldn't it be good to re-open it?

@OlgaMaciaszek
Copy link
Contributor Author

Hello @daspilker Is this PR fine now? Could it be merged?

@daspilker daspilker merged commit 06dda96 into jenkinsci:master Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants