Add an option to load the PowerShell profile #130
Conversation
| @@ -48,6 +48,7 @@ | |||
| public final class PowershellScript extends FileMonitoringTask { | |||
| private final String script; | |||
| private String powershellBinary = "powershell"; | |||
| private boolean loadProfile = false; | |||
There was a problem hiding this comment.
false is the default
| private boolean loadProfile = false; | |
| private boolean loadProfile; |
| @@ -63,6 +64,11 @@ public void setPowershellBinary(String powershellBinary) { | |||
| this.powershellBinary = powershellBinary; | |||
| } | |||
|
|
|||
| public boolean isLoadProfile() { return loadProfile; } | |||
There was a problem hiding this comment.
use line breaks, to match the code style in the file
| public boolean isLoadProfile() { return loadProfile; } | |
| public boolean isLoadProfile() { | |
| return loadProfile; | |
| } |
| public boolean isLoadProfile() { return loadProfile; } | ||
|
|
||
| @DataBoundSetter | ||
| public void setLoadProfile(boolean loadProfile) { this.loadProfile = loadProfile; } |
There was a problem hiding this comment.
use line breaks, to match the code style in the file
| public void setLoadProfile(boolean loadProfile) { this.loadProfile = loadProfile; } | |
| public void setLoadProfile(boolean loadProfile) { | |
| this.loadProfile = loadProfile; | |
| } |
| @@ -7,4 +7,7 @@ | |||
| <f:entry field="powershellBinary" title="Executable"> | |||
| <f:select /> | |||
| </f:entry> | |||
| <f:entry field="loadProfile" title="Load Profile"> | |||
There was a problem hiding this comment.
you could also help add text by creating a file called:
help-loadProfile.html in the same directory
only if you think it's needed though
There was a problem hiding this comment.
I think it's rather obvious, what this switch does. So I would go without a help text. But thanks for the pointer!
ofosos
force-pushed
the
add-profile-loading
branch
from
e15b326
to
b98dcb2
Compare
|
Hey, @timja I incorporated your changes. |
|
Looks fine to me. I think you want to add some profile loading tests in https://github.com/jenkinsci/durable-task-plugin/blob/master/src/test/java/org/jenkinsci/plugins/durabletask/PowerShellCoreScriptTest.java |
|
Hey @car-roll, I added to test to see if the flag is present in the current process. That's not really nice, but it looks better than changing profiles on developer machines ;) Is this sufficient for you? |
|
@ofosos admittedly I don't really know about PowerShell and profiles. Is the test somehow validating that the profile has been successfully loaded? I just see assertions for script launch success. |
|
@car-roll The thing is, afaik we can't check that the profile has been loaded. Unless we modify the PowerShell profile and say include a variable. However it's kind of bad to modify every developers profile. So what we do instead is this: We spin up a powershell and the PowerShell script checks the flags of the current process (the powershell process) and returns success iff this includes the -NoProfile option. We check that the normal state is success and the return code is failure, when we pass the option to load the profiles. |
|
@car-roll ping |
|
@timja looks like your comments were all addressed in this PR. Anything else you would like to add? |
|
Lgtm |
Hey Team,
I would like to be able to load the PowerShell profile on the agent. This change is a step in that direction.
Please be kind. I ran the unit tests, but I'm not sure this will work, any ideas on how to test this properly?
Cheers, Mark