[added] proper Pipeline step for retrieving Build User vars

[awittha]

This plugin could be used with Jenkins Pipeline code via the wrap(){} utility, e.g.

Jenkinsfile:

wrap([$class: 'BuildUser']) {
	def user_email = env["BUILD_USER_EMAIL"]
}

Being restricted to operating within the wrap block made some pipeline scripts difficult to construct.

This PR introduces a getBuildUser() Jenkins pipeline step so that pipeline code can more easily access the information:

Jenkinsfile:

def user_email = getBuildUser().BUILD_USER_EMAIL

The Jenkins Pipeline "Snippet Generator" correctly describes & generates this step:

[awittha]

I use Eclipse, so I needed this.

[martinda]

You can also save personal preferences in ~/.config/git/ignore

[awittha]

Would you like me to remove these .gitignore changes from this Pull Request? Seeing how vim editor-specific excludes were already there, I tried to follow what appeared to be the project's existing pattern.

[martinda]

Oh, I added the vim ignore in there... I did not know about ~/.config/git/ignore back then. You could remove the vim ones too. It makes sense to keep the Maven one since maven is specific to this project. It's not critical though.

[unlikelyzero]

Any chance this'll get merged?

[awittha]

I have removed editor-specific excludes from the local .gitignore.

[fabiodcasilva]

Hey @awittha, @bozaro, @martinda,

I'm really interested in merging this functionality to the plugin and include it in the next release. Is there any way that I can help this move forward?

[awittha]

I think it had to run on the agent (require FilePath.class) because when setting the properties as envvars, it tries to set the env that'll be on an agent, which doesn't work if there is no agent. However, it has been a long time since I had time to touch this, so I am not sure.

Someone could build this branch with the FilePath.class requirement removed, and see if the Build User vars can be successfully set in both of the following situations:

1. inside a node(){...} block on in a scripted pipeline
2. when there is no agent provisioned at all (scripted or declarative pipeline)

If so, then that change can be committed and that would resolve all of the PR feedback.

[fabiodcasilva]

I think it had to run on the agent (require FilePath.class) because when setting the properties as envvars, it tries to set the env that'll be on an agent, which doesn't work if there is no agent. However, it has been a long time since I had time to touch this, so I am not sure.

Someone could build this branch with the FilePath.class requirement removed, and see if the Build User vars can be successfully set in both of the following situations:

1. inside a node(){...} block on in a scripted pipeline
2. when there is no agent provisioned at all (scripted or declarative pipeline)

If so, then that change can be committed and that would resolve all of the PR feedback.

Hi,

I did the change and the tests. I would like someone to review the pull request again so that we can release it. Thanks

[martinda]

Thank you @fabiodcasilva

I think I lack the knowledge to review this properly. For example I am not able to trace how getBuildUser can be be called from a Pipeline script like so: def user_email = getBuildUser().BUILD_USER_EMAIL (how does it deference BUILD_USER_EMAIL?)

In any case my lack of understanding should not get in the way. I'd like to make sure the new feature has unit tests.

I suggest the following:

1. Update the README with the usage information provided in the first comment
2. Provide unit testing of the new feature

[fabiodcasilva]

Thank you @fabiodcasilva

I think I lack the knowledge to review this properly. For example I am not able to trace how getBuildUser can be be called from a Pipeline script like so: def user_email = getBuildUser().BUILD_USER_EMAIL (how does it deference BUILD_USER_EMAIL?)

In any case my lack of understanding should not get in the way. I'd like to make sure the new feature has unit tests.

I suggest the following:

1. Update the README with the usage information provided in the first comment
2. Provide unit testing of the new feature

@martinda I added the necessary unit tests and changes to the documentation. Please check that everything is as expected.

[martinda]

Thanks @fabiodcasilva.

I am doing some tests. When I load the plugin in a manual install based on Jenkins core 2.164.3, I get the following error:

Sep 13, 2020 1:23:49 PM hudson.model.UpdateCenter$DownloadJob run
SEVERE: Failed to install SAML
java.io.IOException: Failed to dynamically deploy this plugin
	at hudson.model.UpdateCenter$InstallationJob._run(UpdateCenter.java:2034)
	at hudson.model.UpdateCenter$DownloadJob.run(UpdateCenter.java:1726)
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at hudson.remoting.AtmostOneThreadExecutor$Worker.run(AtmostOneThreadExecutor.java:112)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.io.IOException: Failed to install saml plugin
	at hudson.PluginManager.dynamicLoad(PluginManager.java:916)
	at hudson.PluginManager.dynamicLoad(PluginManager.java:852)
	at hudson.model.UpdateCenter$InstallationJob._run(UpdateCenter.java:2030)
	... 5 more
Caused by: java.io.IOException: SAML Plugin version 1.1.7 failed to load.
 - You must update Jenkins from version 2.164.3 to version 2.176.1 or later to run this plugin.
	at hudson.PluginWrapper.resolvePluginDependencies(PluginWrapper.java:821)
	at hudson.PluginManager.dynamicLoad(PluginManager.java:906)
	... 7 more

The minimal core version required appears to be 2.176.1, but how to discover that with unit tests is a mystery to me.

[martinda]

First, I am happy you are helping @fabiodcasilva . You forced me to spend a few hours refreshing my memory on plugin authoring, this is good!

After playing with it quite a bit, I think the build step needs to fail if the user attempts to access a variable that the step does not provide. Otherwise, there is a silent failure, which might take a long time to notice.

[martinda]

By definition, a Map returns null when the key is non existent (see get(Object key). This means that the users who make a typo will have silent failures, e.g. getBuildUser().BULD_USER_ID will return the string null.

I'd prefer an outright failure.

[fabiodcasilva]

@martinda I understand your concern and agree that an outright failure would be a way to smooth out errors such as typos. But, after thinking about it for a while, I'm not sure how it can be done in this case. Do you have any ideas or suggestions?
Thanks!

[fabiodcasilva]

@jenkinsci/code-reviewers can someone provide a suggestion/approach to overcome the issue presented above?

[KIVagant]

The most desired PR on GitHub, lol.

[faroguy]

Would love for this to be released. Any progress?

[jglick]

This seems like overkill to me. Is there some reason this plugin is not just defining an EnvironmentContributor?

[basil]

This seems like overkill to me. Is there some reason this plugin is not just defining an EnvironmentContributor?

Done in #24

[fabiodcasilva]

This seems like overkill to me. Is there some reason this plugin is not just defining an EnvironmentContributor?

Done in #24

Closing this PR since #24 is now merged.