[ISSUE-104] fix bug: `cyphernetes shell -A` results in `panic`

[jameskim0987]

Fixes #104

Changes made in this PR:

1. Fix panic for cyphernetes shell -A
2. Prompt the red ALL NAMESPACES shellprompt after starting shell
3. [ TODO ]: add tests

[AvitalTamir]

Just looked at the actions output and seems like theres an issue with the workflow - the test is failing but the pipeline step passes. Will take a look at this.

[jameskim0987]

Hi @AvitalTamir, I have some updates on the test issue -- I think I'm halfway there understanding the core of the issue.

Try the following so we are on the same page:

1. In cmd/cyphernetes/main_test.go, move this test TestCyphernetesShellWithHelpFlag up top so it is the first test in main_test.go
2. Run the tests
3. Now you should be able to observe that the subsequent tests that involve cyphernetes shell ... which are TestCyphernetesShellNoFlag, TestCyphernetesShellWithAllNamespacesFlag, and TestCyphernetesShellWithNamespaceFlag all fail.
4. Now if you move TestCyphernetesShellWithHelpFlag in between those 3 failed tests, then the tests after TestCyphernetesShellWithHelpFlag fails. This shows that TestCyphernetesShellWithHelpFlag is the main issue.

After analyzing the code and debugging through the tests, one thing that stood out to me was that after TestCyphernetesShellWithHelpFlag, the subsequent tests were using the same instance of rootCmd ( which is a pointer to cobra.Command ). Apart from it being a global variable, another way I verified out about this was, on the subsequent tests, the rootCmd object contained a non-nil value for c.helpCommand ( c.helpCommand is initialized through c.InitDefaultHelpCmd() and c.InitDefaultHelpFlag() ). I also directly printed out the address of rootCmd and the same address was being used across tests.

and because that "help" flag remained as part of the Command from TestCyphernetesShellWithHelpFlag and the same Command was being referenced from subsequent tests, the subsequent tests were constantly returning true for helpVal and handling the execution there as if the arguments were "cyphernetes shell --help".

ref: https://github.com/spf13/cobra/blob/main/command.go#L898-L907

Now, going back to the tests in main_test.go, I have some brainstorming questions:

1. Since we are invoking main() for each test, does it share any objects without a clean teardown across tests?
2. How could we make the tests such that we avoid objects being shared explicitly / implicitly across tests ( ex: rootCmd )?

My initial thought is, instead of declaring and initializing rootCmd in global scope, could we try declaring rootCmd in global but initialize it in init()? ( I'm trying to apply it this way... but giving me nil pointers from operator.go init(), will keep trying )

I'm quite positive that this is the crux of the issue but not sure what would be the best approach to solve it. Let me know your thoughts.

[AvitalTamir]

Edit: Read what I answered below but come to think of it - I think we can just lose the help flag test.

@jameskim0987 thanks for catching this! I looked around for a way to tear down or at least clear the flags but didn't manage - so to work around it I tried sandboxing each test in it's own go execution, it's not beautiful but it works. What do you think? https://github.com/AvitalTamir/cyphernetes/blob/issue-104-fix-shell-A-flag/cmd/cyphernetes/main_test.go

(I also addressed the pipeline issue in this branch - It was the Makefile piping test outputs to sed... Removed it.)

Let me know what you think.

(PS I think this approach works but the code is iffy to work with... maybe we can just lose the test with help flag? This is a cobra generated flag and output anyway, is it worth testing on our end?)

[jameskim0987]

@AvitalTamir I agree that we can lose the check for the help flag. I was actually on the fence about whether we should test it or not. Initially I thought we should cover it for the sake of completeness but since it's auto generated I think it is safe to drop it ( unless in the future we decide to add custom help output ).

Looking at your sandboxed and refactored test code though, I think it is ok to leave it since it's already there.

Side note, from the tests I had, when I removed the help check test and ran the entire tests, TestExecuteNoArgs was failing so I think we should keep your test changes.

In short, looks good to me!

[AvitalTamir]

Great, same here! You can mark it ready for review and I'll merge :)

[AvitalTamir]

Thanks @jameskim0987! This is very exciting!

I enjoyed working with you on this. Thanks again for reporting, and resolving this issue 🔥

[jameskim0987]

@AvitalTamir Likewise! Finding and resolving bugs is pretty exciting and thanks for your supervision. I'm glad to contribute!