Allow setting pwsh as powershell executable #111

jetersen · 2019-10-07T01:28:25Z

Upstream of jenkinsci/workflow-durable-task-step-plugin#122
added docker fixture test for pwsh.

This should allow us to create a pwsh step in workflow-durable-task-step

resolves #55
resolves #73
resolves #88

jetersen · 2019-10-07T02:03:42Z

cc @slide @dwnusbaum

also atm PowerShellScript does not show up in the freestyle job.

Tested both on master and this branch.

car-roll

LGTM, but I haven't really worked much on the Powershell side really, ~~so I don't feel comfortable giving approval~~ actuallly, nvm, looking it over again I think I was just being overly cautious :)

jetersen · 2019-10-08T03:40:18Z

cc @jglick @gabloe @jtnord perhaps you want to review my changes?
I added a dedicated pwsh step instead of allowing the powershell step to choose edition. jenkinsci/workflow-durable-task-step-plugin#122

jtnord · 2019-10-08T09:55:44Z

src/main/java/org/jenkinsci/plugins/durabletask/PowershellScript.java

@@ -106,8 +117,8 @@ public String getScript() {
        // Copy the helper script from the resources directory into the workspace
        c.getPowerShellHelperFile(ws).copyFrom(getClass().getResource("powershellHelper.ps1"));

-        if (launcher.isUnix()) {
-            // There is no need to add a BOM with Open PowerShell
+        if (launcher.isUnix() || "pwsh".equals(powershellBinary)) {


FYI: many launchers always say true and lie :-( (but this is existing issue)

The binary is pwsh.exe (and on windows is normally case insensitive - so this should allows pwsh pwsh.exe and all cases of that if I understand the intent here)

you do not have to specify pwsh.exe, pwsh is fine.
@jtnord this is existing logic that been working for quite awhile

see existing code

durable-task-plugin/src/main/java/org/jenkinsci/plugins/durabletask/PowershellScript.java

Lines 86 to 96 in 93b86c9

// Note: PowerShell core is now named pwsh. Workaround this issue on *nix systems by creating a symlink that maps 'powershell' to 'pwsh'.

String powershellBinary = "powershell";

String powershellArgs;

if (launcher.isUnix()) {

powershellArgs = "-NoProfile -NonInteractive";

} else {

powershellArgs = "-NoProfile -NonInteractive -ExecutionPolicy Bypass";

}

args.add(powershellBinary);

args.addAll(Arrays.asList(powershellArgs.split(" ")));

args.addAll(Arrays.asList("-Command", cmd));

you do not have to - but it is generally good practice to do so.
thus if you follow good practices then this will not work

despite this being a String it is not currently user editable from jelly so I guess this is a moot point

jetersen · 2019-10-08T22:10:16Z

@dwnusbaum I'd appreciate you casting a review, so if there is anything that needs changing. That would be favorable 😸

dwnusbaum

@Casz thanks for the PR and for adding tests! I am not really familiar with PowerShell (Core), but given @gabloe's approval I trust that this makes sense in general.

dwnusbaum · 2019-10-09T14:27:37Z

src/main/java/org/jenkinsci/plugins/durabletask/PowershellScript.java

+
+    @DataBoundSetter
+    public void setPowershellBinary(String powershellBinary) {
+        this.powershellBinary = powershellBinary;


Would it make sense to do some validation here, or do you intentionally want to let users specify anything in case they've set up a symlink or something?

The UI only allows two options however from API it could by anything.

Do you see an issue with that?

Looking into it some more, since jenkinsci/workflow-durable-task-step-plugin#122 is the only way users would be exposed to the change, and they wouldn't specify the actual binary, just the new pwsh step, I don't think validation is necessary.

It seems a bit confusing that we even have Jelly files in this plugin, since IIUC they are not currently used anywhere, because the steps in workflow-durable-step define their own config and help files?

The jelly files here should appear in FreeStyle but as I wrote in the PR description. This does not currently work for master branch nor this PR.

Should I try and do a git bisect to determine when PowerShell option disappeared from Jelly view?

I thought that feature was provided by this plugin: https://github.com/jenkinsci/powershell-plugin?

Sure, do you mind if I switch the powershell and pwsh to public static final string or do you prefer enum?

I'm not sure what you mean, I think the code in the PR is good as-is, no changes necessary. I'm happy to merge it once the CI build passes.

It looks like the CI build is unavailable? Getting a 503 page.

Yeah, I think ci.jenkins.io is down right now (unfortunately a common occurrence these days). Looks like the infra team is investigating based on #jenkins-infra on IRC.

src/main/resources/org/jenkinsci/plugins/durabletask/PowershellScript/config.jelly

Co-Authored-By: Devin Nusbaum <dwnusbaum@users.noreply.github.com>

jetersen · 2019-10-09T20:29:50Z

Triggering CI 😰

jetersen · 2019-10-10T15:35:04Z

Ci passed

jetersen · 2019-10-11T05:25:58Z

🚢 📦 🚀

jetersen · 2019-10-21T08:09:08Z

I'd really like for pwsh step to be available. So I would appreciate a release 😓

car-roll · 2019-10-22T16:58:38Z

@Casz Looks like master is having an issue right now, hopefully it's just infra issues (unable to build windows image for tests). Once I get those resolved I'll get a release out.

jetersen added 2 commits October 7, 2019 02:53

fix test, need to launch with -NoProfile

0bd65e2

allow setting powershell binary

ea25e8e

This was referenced Oct 7, 2019

Added powershell/pwsh detection [JENKINS-48803] #88

Closed

add pwsh step jenkinsci/workflow-durable-task-step-plugin#122

Merged

use native or docker

e576f2b

slide approved these changes Oct 7, 2019

View reviewed changes

dwnusbaum requested review from dwnusbaum and car-roll October 7, 2019 16:19

car-roll reviewed Oct 7, 2019

View reviewed changes

car-roll approved these changes Oct 7, 2019

View reviewed changes

gabloe approved these changes Oct 8, 2019

View reviewed changes

jtnord reviewed Oct 8, 2019

View reviewed changes

jtnord approved these changes Oct 8, 2019

View reviewed changes

jetersen mentioned this pull request Oct 9, 2019

Features/windows agents with pathing issues fixed jenkinsci/docker-workflow-plugin#184

Merged

dwnusbaum approved these changes Oct 9, 2019

View reviewed changes

spelling

32cfda0

Co-Authored-By: Devin Nusbaum <dwnusbaum@users.noreply.github.com>

jetersen closed this Oct 9, 2019

jetersen reopened this Oct 9, 2019

dwnusbaum merged commit 0fca597 into jenkinsci:master Oct 10, 2019

jetersen deleted the fix/pwsh branch October 11, 2019 05:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting pwsh as powershell executable #111

Allow setting pwsh as powershell executable #111

jetersen commented Oct 7, 2019 •

edited

Loading

jetersen commented Oct 7, 2019

car-roll left a comment •

edited

Loading

jetersen commented Oct 8, 2019

jtnord Oct 8, 2019

jetersen Oct 8, 2019 •

edited

Loading

jtnord Oct 8, 2019

jtnord Oct 8, 2019

jetersen commented Oct 8, 2019

dwnusbaum left a comment

dwnusbaum Oct 9, 2019

jetersen Oct 9, 2019

dwnusbaum Oct 9, 2019

jetersen Oct 9, 2019

dwnusbaum Oct 9, 2019

jetersen Oct 9, 2019

dwnusbaum Oct 9, 2019

jetersen Oct 9, 2019

gabloe Oct 9, 2019 •

edited

Loading

dwnusbaum Oct 9, 2019

jetersen commented Oct 9, 2019

jetersen commented Oct 10, 2019

jetersen commented Oct 11, 2019

jetersen commented Oct 21, 2019

car-roll commented Oct 22, 2019

	// Note: PowerShell core is now named pwsh. Workaround this issue on *nix systems by creating a symlink that maps 'powershell' to 'pwsh'.
	String powershellBinary = "powershell";
	String powershellArgs;
	if (launcher.isUnix()) {
	powershellArgs = "-NoProfile -NonInteractive";
	} else {
	powershellArgs = "-NoProfile -NonInteractive -ExecutionPolicy Bypass";
	}
	args.add(powershellBinary);
	args.addAll(Arrays.asList(powershellArgs.split(" ")));
	args.addAll(Arrays.asList("-Command", cmd));

Allow setting pwsh as powershell executable #111

Allow setting pwsh as powershell executable #111

Conversation

jetersen commented Oct 7, 2019 • edited Loading

jetersen commented Oct 7, 2019

car-roll left a comment • edited Loading

Choose a reason for hiding this comment

jetersen commented Oct 8, 2019

Choose a reason for hiding this comment

jetersen Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetersen commented Oct 8, 2019

dwnusbaum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabloe Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetersen commented Oct 9, 2019

jetersen commented Oct 10, 2019

jetersen commented Oct 11, 2019

jetersen commented Oct 21, 2019

car-roll commented Oct 22, 2019

jetersen commented Oct 7, 2019 •

edited

Loading

car-roll left a comment •

edited

Loading

jetersen Oct 8, 2019 •

edited

Loading

gabloe Oct 9, 2019 •

edited

Loading