Add support for any shell for stack shellinit #921

endorama · 2022-08-09T15:26:45Z

elastic-package stack shellinit output shell code needed to setup the testing environment and connect to the stack instance started via elastic-package stack up.

The current implementation emits POSIX shell code, compatible with sh, zsh and bash. The shellcode breaks on other shells, like fish - that I'm using. There are workarounds (like the awesome babelfish) but I think a built in solution would be preferred.

This PR aims to address this, adding a simple template mechanism that allows to support any shell through a custom shell code template, adds supports for fish shell and a --shell flag on the shellinit command to select the desired shell output.

This change is not breaking, as by default it outputs POSIX compatible shell code.

elasticmachine · 2022-08-09T15:31:36Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2022-10-18T10:06:39.308+0000
Duration: 33 min 53 sec

Test stats 🧪

Test	Results
Failed	0
Passed	850
Skipped	0
Total	850

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

jsoriano

Nice! Maybe this is also a way to support other shells like powershell in the future 🙂

Added some comments, the most important ones would be to decide if we want separated types for the different posix shells, and to define the new flag and their description in cobraext as is done in other modules. The rest of comments are suggestions about code organization.

internal/stack/shellinit/shell/types.go

cmd/stack.go

internal/stack/shellinit/shell/templates.go

internal/stack/shellinit/shell/code.go

internal/stack/shellinit/shell/type.go

andrewkroh

Thinking about UX and I'm wondering it it would be possible attempt determine the shell automatically such that it automatically does the correct thing for the user's environment (e.g. if we detect fish then output in the appropriate fish format).

I think the most reliable and cross-platform means (assuming we need this to work on Windows to output PowerShell format) of detecting the shell would be to get the executable name of the parent process. Like

        // Using github.com/elastic/go-sysinfo.
	parent, err := sysinfo.Process(os.Getppid())
	if err != nil {
		log.Fatal(err)
	}

	parentInfo, err := parent.Info()
	if err != nil {
		log.Fatal(err)
	}

	shell := filepath.Base(parentInfo.Exe)

Reading the SHELL env var could be a simple option, but if a user has launched a subshell explicitly then it will be wrong (e.g. opening fish from bash).

mtojek

Could you please explain what's wrong with the original implementation? I might be missing something. Fish seems to be supported too.

$ elastic-package completion
Generate the autocompletion script for elastic-package for the specified shell.
See each sub-command's help for details on how to use the generated script.

Usage:
  elastic-package completion [command]

Available Commands:
  bash        Generate the autocompletion script for bash
  fish        Generate the autocompletion script for fish
  powershell  Generate the autocompletion script for powershell
  zsh         Generate the autocompletion script for zsh

$ elastic-package completion fish

mtojek · 2022-08-24T12:53:30Z

I misread your intention, thanks @jsoriano for pointing it. I thought it's about completion not shellinit. In this case, I would research the solution proposed by Andrew. You could use SHELL env to detect the shell.

endorama · 2022-09-07T15:23:53Z

I updated the code with Jaime suggestion:

remove the shell package and put everything in stack
updated cobra flag to cobraext
simplified templates to posix and fish

Instead of exposing posix to users, I kept bash, sh, zsh as I think is more ergonomic (as a user I don't have to worry about if my shell is compatible, just which shell I'm using) and I want to dig into automating this.

I would strongly advise against using SHELL, as from the docs, it

shall represent a pathname of the user's preferred command language interpreter

This can create issues when running the command in subshell or automating it and depends on a parameter that may not be configurable (think running this in the CI), leading to "fail first, configure later" scenario that I think are

I like Andrew's approach, just want to double check if you are ok adding elastic/go-sysinfo as a dependency, as is not currently used. If you're ok, I would change default flag value to empty and use that as a signal to trigger the autodetection. I would still allow overriding this with an explicit value.

jsoriano · 2022-09-07T15:43:05Z

cmd/stack.go

 				return cobraext.FlagParsingError(err, cobraext.ProfileFlagName)
 			}

+			s, err := cmd.Flags().GetString(cobraext.ShellInitShellFlagName)


Please use a more meaningful name here.

Suggested change

s, err := cmd.Flags().GetString(cobraext.ShellInitShellFlagName)

shellName, err := cmd.Flags().GetString(cobraext.ShellInitShellFlagName)

fixed in 18364c6 (#921)

jsoriano · 2022-09-07T15:44:09Z

internal/stack/shellinit.go

+	export %s=%s
+	export %s=%s
+	export %s=%s
+	export %s=%s


This is adding spaces before each export right? Can you check the output of elastic-package stack shellinit?

fixed in 7a98775 (#921)

jsoriano · 2022-09-07T15:44:34Z

internal/stack/shellinit.go

+	case "zsh":
+		return posixTemplate
+	default:
+		panic("shell type is unknown, should be one of " + strings.Join(availableShellTypes, ", "))


s comes directly from user input, we shouldn't panic in this case.

fixed in 5a0adf4 (#921)

jsoriano · 2022-09-07T15:46:55Z

I like Andrew's approach, just want to double check if you are ok adding elastic/go-sysinfo as a dependency, as is not currently used. If you're ok, I would change default flag value to empty and use that as a signal to trigger the autodetection. I would still allow overriding this with an explicit value.

I think it would be fine to add this dependency if it helps to have a better UX.

elasticmachine · 2022-09-07T15:55:46Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`35/35`)	💚
Files	66.667% (`86/129`)	👍 0.775
Classes	61.413% (`113/184`)	👍 0.543
Methods	47.451% (`363/765`)	👍 0.145
Lines	30.634% (`3276/10694`)	👍 0.115
Conditionals	100.0% (`0/0`)	💚

endorama · 2022-09-13T11:23:37Z

Added shell detection using the script provided by Andrew in 57cf67f (#921)

jsoriano · 2022-09-14T09:34:02Z

CI failures seem related, go-sysinfo should be added to go mod.

`stack shellinit` command outputs shell code to setup the environment and connect to the local stack instances created by the `stack` command. Before this change this command was only supporting POSIX compatible shells. After this commit the support has been extended so is possible to specify, through a flag, the desired compatibility for it's output.

This removes the `shell` package moving the logic alongside `ShellInit`, removes the `Shell` type and reduce template code to only POSIX compliant and fish shell.

jsoriano

👍

cmd/stack.go

internal/stack/shellinit.go

cmd/stack.go

internal/stack/shellinit_internal_test.go

jsoriano · 2022-10-05T15:48:26Z

cmd/stack.go

+		return "", fmt.Errorf("cannot retrieve information for parent of process %d: %w", ppid, err)
+	}
+
+	shell := filepath.Base(parentInfo.Exe)


Found another corner case.

When running elastic-package with go run, it detects go as the shell:

detected shell: go Error: shellinit failed: cannot get shell init template: shell type is unknown, should be one of bash, fish, sh, zsh exit status 1

This happens when running elastic-package with go run in repositories where this package is in go.mod. This is done in CI in some cases, and for example I use it when trying to reproduce failures in the integrations repository with the same version of elastic-package that is used in CI.

This could be considered a regression, could we do something about this? Maybe it is possible to detect if elastic-package is being run with go run, or we could get the parent of the parent when it is go.

Or maybe another option to avoid regressions, would be to directly return bash and log something to stderr instead of failing when this method cannot detect the shell.

This use case is supported by not using auto detection (passing --shell=bash). I think that is preferable than deciding to fallback to BASH because that can lead to errors that are very hard to troubleshoot and less explanatory.

We may want to make this explicit using a different error message.

Well, let's separate my two comments:

Not breaking the not so rare go run case. Possible workaround for this could be: if found shell is go, try to get the parent of the parent instead.

General solution to don't break unknown cases that work now. I propose to use bash in case of error only to keep the current behavior, where only bash is supported. I agree that it could be more discussed as I don't have more use cases beyond the mentioned ones, and it would be better to have explicit behavior.

Not breaking the not so rare go run case. Possible workaround for this could be: if found shell is go, try to get the parent of the parent instead.

I added this in 12ddc3d (with tests)

General solution to don't break unknown cases that work now. I propose to use bash in case of error only to keep the current behavior, where only bash is supported.

I didn't consider this POV, but it make complete sense, as the behaviour would not change from current implementation. I will change the default case.

cmd/stack.go

endorama · 2022-10-06T11:13:28Z

The CI fails on check-static, complaining that go.mod and go.sum have changes, but make check-static passes on my local machine. Any idea how to solve?

jsoriano · 2022-10-06T11:33:43Z

The CI fails on check-static, complaining that go.mod and go.sum have changes, but make check-static passes on my local machine. Any idea how to solve?

CI uses go 1.19, is it possible that you are using an older version?

jsoriano

Apart of some last nitpicking and the issues with CI and go.mod it LGTM, thanks!

cmd/stack.go

Comments addressed.

Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>

endorama · 2022-10-11T10:35:23Z

CI uses go 1.19, is it possible that you are using an older version?

I'm using go 1.19.1 locally, which is the same as reported in .go-version

jsoriano · 2022-10-11T11:27:02Z

CI uses go 1.19, is it possible that you are using an older version?

I'm using go 1.19.1 locally, which is the same as reported in .go-version

If I run make check-static locally with the code of this branch, with go 1.19.2, I see the same errors as in CI. If I run go mod tidy, I see that go.mod and go.sum get changes. 🤔

Shall I push the changes I see to this branch?

endorama · 2022-10-13T08:54:51Z

@jsoriano yes please, I cannot reproduce even with go 1.19.2 :(

endorama · 2022-10-19T10:32:30Z

Thank you!

jsoriano requested a review from a team August 9, 2022 16:04

jsoriano requested changes Aug 9, 2022

View reviewed changes

jsoriano added enhancement New feature or request Team:Ecosystem Label for the Packages Ecosystem team labels Aug 9, 2022

jsoriano assigned endorama Aug 9, 2022

andrewkroh reviewed Aug 17, 2022

View reviewed changes

mtojek self-requested a review August 22, 2022 12:02

mtojek previously requested changes Aug 22, 2022

View reviewed changes

jsoriano requested changes Sep 7, 2022

View reviewed changes

jsoriano self-requested a review September 13, 2022 15:10

endorama added 12 commits September 27, 2022 16:25

add stack/shellinit/shell package

01eccf4

move everything within stack package

1014837

This removes the `shell` package moving the logic alongside `ShellInit`, removes the `Shell` type and reduce template code to only POSIX compliant and fish shell.

use cobraext for shell init flag

5e36eb3

add license header

fc39daf

remove space

789ec84

proper error handling

73641da

better var name

6a78451

implement shell automatic detection

a681152

print detected shell to stderr

7abf019

fixup! better var name

927cd40

update go.mod

a3e839b

jsoriano reviewed Sep 27, 2022

View reviewed changes

endorama added 5 commits October 5, 2022 15:09

move detect value to constant

8fbaf11

trim eventual .exe extension

44f2f47

use long var name

9eddd76

fix tests

3bfc722

go mod tidy

a90bef7

jsoriano reviewed Oct 5, 2022

View reviewed changes

endorama added 2 commits October 6, 2022 12:30

go import

5681952

support special detected shell: go

12ddc3d

jsoriano approved these changes Oct 6, 2022

View reviewed changes

cmd/stack.go Show resolved Hide resolved

cmd/stack.go Outdated Show resolved Hide resolved

Update cmd/stack.go

fa3c05c

Co-authored-by: Jaime Soriano Pastor <jaime.soriano@elastic.co>

Try to fix go.mod

6953f5b

jsoriano approved these changes Oct 13, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into shellinit-fish-support

63ae264

jsoriano merged commit 88c16e0 into elastic:main Oct 18, 2022

endorama deleted the shellinit-fish-support branch October 19, 2022 10:29

	s, err := cmd.Flags().GetString(cobraext.ShellInitShellFlagName)
	shellName, err := cmd.Flags().GetString(cobraext.ShellInitShellFlagName)

Add support for any shell for stack shellinit #921

Add support for any shell for stack shellinit #921

Uh oh!

Conversation

endorama commented Aug 9, 2022

Uh oh!

elasticmachine commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

mtojek commented Aug 24, 2022

Uh oh!

endorama commented Sep 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsoriano commented Sep 7, 2022

Uh oh!

elasticmachine commented Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

Uh oh!

endorama commented Sep 13, 2022

Uh oh!

jsoriano commented Sep 14, 2022

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsoriano Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

endorama commented Oct 6, 2022

Uh oh!

jsoriano commented Oct 6, 2022

Uh oh!

elasticmachine commented Aug 9, 2022 •

edited

Loading

elasticmachine commented Sep 7, 2022 •

edited

Loading

jsoriano Oct 5, 2022 •

edited

Loading