Shell command works for complex command

[olblak]

Fix #942

As suggested in issue #942, I am copying the command to a file and then run /bin/sh on that file.

This pullrequest introduces a new temporary directory named /tmp/updatecli/bin used to store shell scripts created by Updatecli.
when the shell resource is triggered, we generated a file where the name is the hash of the command

Manifest used during testing

sources:
  demo:
    name: Test complex command
    kind: shell
    spec:
      command: |
          curl -s https://api.github.com/repos/updatecli/updatecli/releases | grep tag_name | cut -d "\"" -f 4| sort -V | tail -n1

which generate the file /tmp/updatecli/bin/223f8bfb9f09e60649fe1dd60e59fad7

Test

To test this pull request, you can run the following commands:

cp <to_package_directory>
go test

Additional Information

By default the script is not deleted unless the updatecli uses the flag --clean=true such as updatecli diff --config manifest.yaml --clean=true

Tradeoff

Potential improvement

A potential improvement would be to specify the shell command to use. By default /bin/sh sounds the best minimal one.

[olblak]

A positive side effect of this pullrquest is that it allow us to move custom scripts directly in the manifest.

This is a short term workaround for the chicken and egg problem where we write an Updatecli manifest with a script to update a target repository but the manifest can't work because the target repository do not have yet the script...

[dduportal]

The unit test does not work on macOS (darwin):

--- FAIL: TestShell_New (0.00s)
    --- FAIL: TestShell_New/Inherit_PATH_environment_variable (0.00s)
        /Users/dadou/workspace/updatecli/updatecli/pkg/plugins/resources/shell/main_test.go:154: 
            	Error Trace:	/Users/dadou/workspace/updatecli/updatecli/pkg/plugins/resources/shell/main_test.go:154
            	Error:      	Not equal: 
            	            	expected: &shell.Shell{executor:(*shell.nativeCommandExecutor)(0x1005bd660), spec:shell.Spec{Command:"echo Hello", Environments:shell.Environments{shell.Environment{Name:"PATH", Value:""}}, Shell:""}, result:shell.commandResult{ExitCode:0, Stdout:"", Stderr:"", Cmd:""}, interpreter:"/bin/sh"}
            	            	actual  : &shell.Shell{executor:(*shell.nativeCommandExecutor)(0x1005bd660), spec:shell.Spec{Command:"echo Hello", Environments:shell.Environments{shell.Environment{Name:"PATH", Value:""}}, Shell:""}, result:shell.commandResult{ExitCode:0, Stdout:"", Stderr:"", Cmd:""}, interpreter:"/bin/bash"}
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -19,3 +19,3 @@
            	            	  },
            	            	- interpreter: (string) (len=7) "/bin/sh"
            	            	+ interpreter: (string) (len=9) "/bin/bash"
            	            	 })
            	Test:       	TestShell_New/Inherit_PATH_environment_variable

=> This PR is legit but the code (and tests) in its current state is too fragile.

First superficial review:

• I would introduce an ExecuteScript() method on the nce receiver instead of adding more code in the current ExecuteCommand(). Remember the UNIX adage: 1 method should do 1 thing but do it well.
• I would use /bin/sh for any "non Windows" OS: simpler to implement (2 conditional branches instead of 4 with the switch/caseyou have today)

[olblak]

I would introduce an ExecuteScript() method on the nce receiver instead of adding more code in the current ExecuteCommand(). Remember the UNIX adage: 1 method should do 1 thing but do it well.

Good suggestion, I'll try to find some time to improve this

I would use /bin/sh for any "non Windows" OS: simpler to implement (2 conditional branches instead of 4 with the switch/caseyou have today)

I have fine with this suggestion, the shell can be overridden anyway so 🤷🏾

This PR is legit but the code (and tests) in its current state is too fragile.
I am not sure if you mean my suggestion or the global shell resource. The tests are failing because we do not mock the OS. so the tests are passing on a Linux machine

If I manage to install gcc on a Windows I would also like to test to see how it behave there

[olblak]

While running some tests, I noticed that result are not what I would expect:

- COMMAND.YAML:
	Source:
		✔ [1] Should be succeeding (kind: shell)
		✗ [2] Should be failing (kind: shell) <- Should be warning
		⚠ [3] Should trigger an error (kind: shell) <- should be error
	Condition:
		✔ [1] Should be succeeding (kind: shell)
		✗ [2] Should be failing (kind: shell)  <- should be warning
		✔ [3] Should trigger an error (kind: shell) <- Should be error

But I am planning to fix this behavior in #1071

[olblak]

@dduportal I think I addressed your concerned.
Before merging I am planning to test this PR on a windows machine.

If the PR is in a acceptable state for you, then I'll merge it so I can continue working on #1071

[olblak]

I identify a few regression

[olblak]

Worth mentioning.
Every shell command is copied to a file like /tmp/updatecli/bin/6b23ffcb257a8674ab6fa1d4f42af96034576d03aea7ebc71840f8355a6ac215.sh I decided to not delete the file after running the command instead we delegate that task to the the flag used at the command level such as.

updatecli diff --clean=true

This allows us to debug the generated file

[olblak]

I did some testing on a windows machine and it seems to work as expected.
I am waiting for feedbacks from a macOS device

[dduportal]

Still failing with macOS. i'm pasting the output here of the condition_test.go file without further diagnostic (I'm digging but I want to share the feedback as soon as posible):

Running tool: /Users/dadou/.asdf/shims/go test -timeout 30s -run ^(TestShell_Condition|TestShell_ConditionFromSCM)$ github.com/updatecli/updatecli/pkg/plugins/resources/shell

time="2023-01-16T14:57:32+01:00" level=info msg="The shell 🐚 command \"\" ran successfully with the following output:\n----\nHello\n----"
time="2023-01-16T14:57:32+01:00" level=info msg="The shell 🐚 command \"\" exited on error (exit code 1) with the following output:\n----\n----\n\ncommand stderr output was:\n----\nls: 1.2.3: No such file or directory\n----"
time="2023-01-16T14:57:32+01:00" level=info msg="The shell 🐚 command \"\" ran successfully with the following output:\n----\nHello\n----"
--- FAIL: TestShell_ConditionFromSCM (0.00s)
    --- FAIL: TestShell_ConditionFromSCM/Successful_Condition_in_existing_SCM (0.00s)
        /Users/dadou/workspace/updatecli/updatecli/pkg/plugins/resources/shell/condition_test.go:134: 
            	Error Trace:	/Users/dadou/workspace/updatecli/updatecli/pkg/plugins/resources/shell/condition_test.go:134
            	Error:      	Not equal: 
            	            	expected: "/bin/bash /tmp/updatecli/bin/9adca274a8e9298dd9060a3bd68649e74950b670b9bc4efe47443e1843091683.sh"
            	            	actual  : "/bin/bash /var/folders/2s/09szbrgn22l2tcvxz1_k34g40000gn/T/updatecli/bin/9adca274a8e9298dd9060a3bd68649e74950b670b9bc4efe47443e1843091683.sh"
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1 +1 @@
            	            	-/bin/bash /tmp/updatecli/bin/9adca274a8e9298dd9060a3bd68649e74950b670b9bc4efe47443e1843091683.sh
            	            	+/bin/bash /var/folders/2s/09szbrgn22l2tcvxz1_k34g40000gn/T/updatecli/bin/9adca274a8e9298dd9060a3bd68649e74950b670b9bc4efe47443e1843091683.sh
            	Test:       	TestShell_ConditionFromSCM/Successful_Condition_in_existing_SCM
FAIL
FAIL	github.com/updatecli/updatecli/pkg/plugins/resources/shell	0.357s
FAIL

[olblak]

@dduportal would you have some time to give it another try? Tests should pass now

[olblak]

I am going to merge this one as it's a requirement for a few open PR