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

[JENKINS-34002] introduce requirePlugins step #31

Closed
wants to merge 3 commits into from

Conversation

@batmat
Copy link
Member

batmat commented Jan 14, 2017

I started looking into this before finally finding JENKINS-34002.

Initially, I thought I would contribute this into pipeline-utility-steps, but then I thought this is really a core step IMO: because having to make sure some non compulsory plugin is installed, to check for some other plugins presence would somehow be defeating the purpose.

Being able to simply express some plugins requirements, and fail fast if absent, as the first instruction of a pipeline would provide great value (compared to the low complexity of this code).

Usage:

requirePlugins 'workflow-step-api@2.4,pipeline-utility-steps' // comma or space supported
@batmat batmat force-pushed the batmat:JENKINS-34002 branch from e58401a to 386d847 Jan 14, 2017
@batmat batmat force-pushed the batmat:JENKINS-34002 branch from 386d847 to c3e190c Jan 14, 2017
@Vlatombe
Copy link
Member

Vlatombe commented Jan 14, 2017

Really a great idea! I love it

Copy link
Member

jglick left a comment

@svanoort is working on the dependency issue, which should allow Pipeline Syntax to suggest a plugin list based on those steps or step configuration subobjects actually used in the last completed build.

private final String plugins;

@DataBoundConstructor
public RequirePluginsStep(String plugins) {

This comment has been minimized.

Copy link
@jglick

jglick Jan 14, 2017

Member

prefer a list

This comment has been minimized.

Copy link
@batmat

batmat Jan 14, 2017

Author Member

I don't feel that strongly, so will do as you prefer, but I chose that on purpose. It just felt to me a wee bit more cumbersome to type "square bracket quote plugin name quote comma quote plugin name quote square bracket" than "quote plugins I need separated by comma/space quote" :-)

This comment has been minimized.

Copy link
@batmat

batmat Jan 15, 2017

Author Member

BTW, trying to find an example of how to handle arrays/list for the Snippet Generator, any pointer? Thanks

This comment has been minimized.

Copy link
@batmat

This comment has been minimized.

Copy link
@svanoort

svanoort Jan 15, 2017

Member

@jglick I think you may have two stories confuse here - I'm not working on the dependencies bit, but the StepAction, so if Batmat is doing something that replaces it, 100% thrilled here :)

This comment has been minimized.

Copy link
@HRMPW

HRMPW Jan 15, 2017

JENKINS-31582 is assigned to you @svanoort but I don't think it is in-progress. AFAIK, what @batmat is doing here only allows the users to manually define what plugins need to be available. This would make Jenkinsfiles more portable but it is still manual and doesn't replace JENKINS-31582. When we get to JENKINS-31582 we can make it works with this.

version = versioned[1];
}

final Plugin plugin = Jenkins.getActiveInstance().getPlugin(pluginId);

This comment has been minimized.

Copy link
@jglick

jglick Jan 14, 2017

Member

I do not think this is the class you want to use. IIRC PluginManager has a better way of getting the PluginWrapper directly. Also you need to check for activation status.

This comment has been minimized.

Copy link
@batmat

batmat Jan 14, 2017

Author Member

Oh right for the activationstatus. Indeed, if the plugin is disabled, then it won't fail right now which would be quite surprising for the user. Will look into PluginManager usage too. Probably tomorrow or so.

@batmat
Copy link
Member Author

batmat commented Jan 15, 2017

@jglick I believe I addressed your two comments:

  • switch to List<String>
  • handle inactive plugins

Also, FWIW I also finally wrote a requireSteps in #33. I am wondering if this way being wouldn't be better for Declarative, WDYT @abayer @HRMPW ?

@HRMPW
Copy link

HRMPW commented Jan 15, 2017

@batmat do you mean both or just this or #33?

Does putting it in syntax checking buy us much, @abayer ? This syntax is dependent on the Jenkins it is running on so valid syntax one Jenkins will fail on a different Jenkins. If I validate my Jenkinsfile on a Jenkins and then try to run it on another it's going to fail at runtime regardless of the syntax. We essentially save the compilation time but the Pipeline would fail quickly anyway. For Declarative Syntax something like this might work well:

pipeline { required { plugins { "docker-pipeline@1.2" } steps { } } }

@batmat
Copy link
Member Author

batmat commented Jan 15, 2017

@HRMPW I mean: maybe requireSteps would be more obvious to newcomers and in general than requirePlugins.

requirePlugins requires more knowledge from user where things are coming from and so on.

@HRMPW
Copy link

HRMPW commented Jan 16, 2017

Honestly, with Declarative we already have explicitly steps sections in each stage. For beginning users it would be much nicer to just collect each steps section and run them through requireSteps for them. This would be much better than making them list all required steps again.

Granted this might not account for script blocks but it would get users pretty far along with no work on their part.

@abayer
Copy link
Member

abayer commented Jan 16, 2017

Actually, that wouldn't work with Declarative - if a step class isn't installed, I can't tell that something like foo("bar") is a step and not a global library function or something like that. I personally like the idea of requirePlugins most, but could see a use for an explicit requireSteps as well.

@HRMPW
Copy link

HRMPW commented Jan 16, 2017

Then the inverse would cause confusion, too. Now, we would require end users to know what is a step and what is a global library. If a user sees all of the steps that are used in his/her pipeline and adds them to the requiredSteps list then the syntax check will immediately fail with "step foo does not exist" and to troubleshoot that error they will have to dig into the fact that "foo" is a global library and not a step.
If we can't tell users what is a step and what is a global library call then either of them is bound to cause confusion. At least with requirePlugins that is a known state, either the plugin is available and enabled or it isn't.

@bitwiseman
Copy link
Contributor

bitwiseman commented Jan 17, 2017

For my part, I would say requirePlugins is better and enough.

Getting too fine-grained will just be annoying. The point of this is (in my mind) is to say, "This Pipeline requires these components", not to say "This Pipeline requires these parts of these components."

When would a user install only part of a plugin?

@batmat
Copy link
Member Author

batmat commented Jan 18, 2017

When would a user install only part of a plugin?

Because IMO most users don't know and probably don't care where steps are coming from. And we're not talking about installing only a part of plugin, but using.

As a user I know which steps I want and use: like findFiles, sh or input.

It's probably harder to find out, and I care less about the plugins those steps are in (and considering they won't move from one plugin to another), which in that case the equivalent would be:

    requirePlugins ["pipeline-utility-steps","workflow-durable-task-step","pipeline-input-step"]
@bitwiseman
Copy link
Contributor

bitwiseman commented Jan 18, 2017

@batmat - Ah, I see. I guess, it depends on the user. I don't know, listing all the steps still seems excessive.

At the top, you described this as a "proposal" to address JENKINS-34002. But this and #33 don't really address that issue in a cohesive way.
They provide two facilities that address some aspects of that issue but they don't provide an overall design for "require"-ing dependencies.

Part of why we'd want this functionality is to improve the clarity and stability of Pipeline code. That means that providing more ways to do this is not necessarily better.

I don't mean to dump on what you've done. It is great to see this getting attention, but just implementing and releasing several ways to do this does improve things. The range of this thread and the question being asked are good examples of that. It seems to me like more design and discussion is needed before we start talking about pull requests for specific implementations.
We should decide on a structure that is understandable and logical, and then incrementally implement that.

What do you think of the ideas in JENKINS-34002?

@jglick @svanoort - You're working on something in the Pipeline Syntax area related to this? Is there an issue tracking that? Is that linked to JENKINS-34002 or separate?

@abayer @HRMPW - Is some form of this "requires" structure part the plan for Declarative? Part of the launch? There's already a tools section in Declarative and we've made the last syntax breaking changes, so if there's a requires section it would not include tools.

@jglick
Copy link
Member

jglick commented Feb 10, 2017

the dependency issue, which should allow Pipeline Syntax to suggest a plugin list

jenkinsci/workflow-api-plugin#26

@svanoort
Copy link
Member

svanoort commented Feb 10, 2017

Once again @jglick I'm unclear how my work on jenkinsci/workflow-api-plugin#26 relates to this?

@jglick
Copy link
Member

jglick commented Feb 28, 2017

I'm unclear how my work on jenkinsci/workflow-api-plugin#26 relates to this?

Because it is very unfriendly to expect users to know exactly which plugins their build used. Sometimes it is obvious, sometimes not. With that API in hand, the Snippet Generator can offer you an initial list of plugin names and versions which the last successful build of the project actually used, at least in step configuration, so you do not have to spend time researching that.

@HRMPW
Copy link

HRMPW commented Feb 28, 2017

@bitwiseman Apologies neither @abayer nor I responded on this before.

A couple of things come to mind here:

  1. I think we should add this to Pipeline in general first and then add sugar for it in Declarative. Having a simple explicit step (requirePlugins) for Scripted Pipeline is probably a cleaner implementation both for using and for maintaining the step. We don't have to try to fit every possible use case into a single thing. We can build whatever we want on top of that for Declarative Pipeline. That could be a requires section a plugins section etc. Again, having clean, granular steps in Scripted is better IMO. Thoughts @abayer

  2. To @jglick 's point it seems like JENKINS-31582 will make this much easier for users to implement on their own without needing to know every plugin specifically used.

  3. I'll add a separate comment on PR-33 regarding steps.

@abayer
Copy link
Member

abayer commented Feb 28, 2017

I pretty much always want a Scripted step to build Declarative sugar on top of, so yeah.

@svanoort
Copy link
Member

svanoort commented Feb 28, 2017

@jglick For the 3rd time: that workflow-api work does not add tracking of plugin & version. It tracks arguments supplied to the Step -- I still do not know why you tried to combine those two items in one implementation JIRA, but the approach taken here does not do so, nor will it.

@batmat
Copy link
Member Author

batmat commented Mar 30, 2017

@HRMPW and all, so how do you feel about this one?

We can close it if there's no plan to merge it. No problem, I learnt a bit about pipeline steps development in the go, so not a total loss for me anyway :-). Just let me know.

@HRMPW
Copy link

HRMPW commented Mar 30, 2017

@batmat I think there is value here. It might be somewhat manual to use right now but we can still implement this step and add some automation and plugin discovery on it later.

@jglick
Copy link
Member

jglick commented May 23, 2017

does not add tracking of plugin & version. It tracks arguments supplied to the Step

But you should be able to reconstruct plugin information from that, no? If not, we will need to amend that API. This is critical for usability of a requirePlugin step.

@HRMPW
Copy link

HRMPW commented May 23, 2017

Is JENKINS-31582 really required here? I'm not opposed to getting JENKINS-31582 done, too, especially if it were integrated with requirePlugins in Snippet Generator. However, it seems like an initial version of this could be done without having an automated list generated. Otherwise, we leave @batmat hanging here until the rest is complete.

@jglick
Copy link
Member

jglick commented May 30, 2017

Pretty awkward without that—you would need to go to /pluginManages/installed, look for plugins which you can recall using in your build somewhere, then copy and paste.

@jglick
Copy link
Member

jglick commented Jun 30, 2017

ArgumentsAction.getResolvedArguments can now be used for this purpose.

@batmat
Copy link
Member Author

batmat commented Mar 14, 2018

Closing it for clarity it's mostly inactive and needs fixes unfortunately I have currently no time to work on. It's linked from the JIRA issue though, so anyone wanting to take over is obviously and warmly welcome.

@batmat batmat closed this Mar 14, 2018
@batmat batmat deleted the batmat:JENKINS-34002 branch Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.